Closed Bug 641254 Opened 13 years ago Closed 13 years ago

file.open() and text-streams/byte-streams docs are confusing

Categories

(Add-on SDK Graveyard :: Documentation, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: asqueella, Assigned: adw)

Details

Attachments

(1 file)

https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/docs/file.md says:

  > @returns {stream} A stream that can be used to access or modify
  > the contents of the file. See text-streams and byte-streams for
  > more information.

while the linked text-streams https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/docs/text-streams.md says:

  > The text-streams module provides streams for reading and writing text 

and then goes on to describe two classes TextReader/TextWriter, which are documented to take an 'inputStream' in their constructor.


The fact that a module claiming to provide "streams" only has constructors taking streams as parameters is interesting, but I didn't pay much attention to it.

The logical thing to do seemed to call TextWriter(file.open(.., "w")), since file.open() returns a stream, and TextWriter takes a stream. That fails in very non-obvious ways:
- Error: The stream is closed and cannot be used.' when calling method: [nsIOutputStream::close]"  nsresult: "0x8057001c (NS_ERROR_XPC_JS_THREW_JS_OBJECT
- Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIBufferedOutputStream.close]"
- [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIBufferedOutputStream.flush]"

I think that:
- file docs should specify the exact types of objects that are returned (TextReader/TextWriter/ByteReader/ByteWriter) and not call them streams.
- text-streams / byte-streams should probably not call themselves streams to avoid confusion with nsI streams. The docs should say that other APIs usually return the Text/ByteReader/Writer object and not the low-level (nsI...) streams.
Apparently I'm not alone in interpreting docs like this: http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/92a0de7a8aca05e9
Attached patch patchSplinter Review
I think this is less confusing than you and the other guy make it out to be.  The text-streams and byte-streams docs specifically say their constructors take nsI streams.  There's no handwaving about it.

I agree that file.open() should describe the types of objects it returns though.  That's what this patch does.

(In reply to comment #0)
> The fact that a module claiming to provide "streams" only has constructors
> taking streams as parameters is interesting, but I didn't pay much attention to
> it.

No, not really.  Streams encapsulating and consuming other streams is pretty commonplace.  Look no further than nsIBufferedInputStream, nsIBinaryOutputStream and other Gecko stream interfaces.
Attachment #519590 - Flags: review?(wbamberg)
Thanks, specifying the exact type of the object returned does make this much less confusing!
Comment on attachment 519590 [details] [diff] [review]
patch

Thanks Drew. That makes sense to me.
Attachment #519590 - Flags: review?(wbamberg) → review+
https://github.com/mozilla/addon-sdk/commit/36af513a23cd6796e5c50fabc3c190829919e9ae
Assignee: nobody → adw
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0b4
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: