161912 - document nsIFile

Status: RESOLVED WORKSFORME

(Core :: XPCOM, defect)

Description

[timeless]

it's @status FROZEN

and look at all of this wonderful documentation:
boolean nsIFile::exists (    )
boolean nsIFile::isWritable (    )
boolean nsIFile::isReadable (    )
boolean nsIFile::isExecutable (    )
boolean nsIFile::isHidden (    )
boolean nsIFile::isDirectory (    )
boolean nsIFile::isFile (    )
boolean nsIFile::isSymlink (    )

There's no indication if any of these methods will throw errors if files don't
exist or other fun things.

Some other gems:
boolean nsIFile::equals (  in nsIFile    inFile )
 Will determine if the inFile equals this. 

That's nice, but it didn't define what equals meant.

boolean nsIFile::contains (  in nsIFile    inFile, in boolean    recur) 
Will determine if inFile is a descendant of this file If |recur| is true, look
in subdirectories too.

I have no idea what this means.

 void nsIFile::createUnique  (   in unsigned long     type,
in unsigned long    permissions
) 
 

createUnique.

This function will create a new file or directory in the file system. Any nodes
that have not been created or resolved, will be. If this file already exists, we
try variations on the leaf name "suggestedName" until we find one that did not
already exist.

If the search for nonexistent files takes too long (thousands of the variants
already exist), we give up and return NS_ERROR_FILE_TOO_BIG.

"did not already exist" um, a file might have been created and deleted in which
case it already existed once.

"the search for /nonexistent/ file*s*" how many are you searching for? you only
need one right?

The idl docs talk about symlinks, aliases and shortcuts. but usually no more
than two at a time. It'd be nice if some sort of consistency was present. I'd
suggest always mentioning all three.

--
moveTo[Native].

This will move this file to the specified newParentDir. If a newName is
specified, the file will be renamed. If 'this' is not created we will return an
error (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST).

*I'm willing to bet that the windows implementation violates this part of the
contract:*
moveTo will NOT resolve aliases/shortcuts during the copy.

/If the windows implementation doesn't violate the contract then it's probably
because the contract doesn't mention symlinks, in which case what you have is
totally useless./

moveTo will do the right thing and allow copies across volumes.

Comment 1

[Doug Turner (:dougt)]

I agree with the bug, but not your sarcasm.  If you want to be helpful, try to
fix the problem and not just bitch about it!

Comment 2

[Sören 'Chucker' Kuklau (gone)]

Attachment: lousy attempt at a patch

Beware. This is my very first patch, so obviously it will suck. I did not
address most of timeless' comments, as I'm not familiar with nsIFile myself and
thus cannot answer all questions posed. This patch is incomplete and I only
upload it so I have an idea whether I'm doing the right thing.

In particular, I commented briefly on all "wonderfully documented" booleans and
reformulated some unclear expressions.

> There's no indication if any of these methods will throw errors

I don't know how to find this out.

> That's nice, but it didn't define what equals meant.

Ditto.

> Will determine if inFile is a descendant of this file If |recur| is true,
> look in subdirectories too.

As far as I can tell, a "descendant of a file" can only be a child of a
directory, so that's how I reformulated it. I may be wrong.

So, you can start flaming me now.

Comment 3

[Sören 'Chucker' Kuklau (gone)]

Comment on attachment 110310 [details] [diff] [review]
lousy attempt at a patch

Will write better version incorporating timeless' suggestions.

Comment 4

[Sören 'Chucker' Kuklau (gone)]

Attachment: second take

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 110316 [details] [diff] [review]
second take

your usage of e.g. seems wrong, you probably mean i.e., in all cases where you
added it.

the documentation for contains, the recur param, to be exact, has odd
indentation; please don't use tabs.

as for finding out if methods throw errors, if they have a |return NS_ERROR_*;|
somewhere, they do.

+     * Checks if the file is a directory.

uh, I'm sure this can be phrased better. s/file/nsIFile/ maybe.

+      *  clone()
Why are you mentioning the name of the function in the comment, you don't do it
for other functions either.

Comment 6

[Sören 'Chucker' Kuklau (gone)]

Attachment: (Hopefully) addressed biesi's comments

Comment 7

[Sören 'Chucker' Kuklau (gone)]

taking

Comment 8

[Sören 'Chucker' Kuklau (gone)]

ditto

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 110321 [details] [diff] [review]
(Hopefully) addressed biesi's comments

You know most of these methods can return many more error codes than just what
you are listing.  For example, in Create() we map the error code with
ConvertWinError(GetLastError());

How do you plan to document these errors?

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

What about listing the errors that ConvertWinError (and other platforms'
equivalents) can return near the top of the idl file, with a comment like "These
errors are possibly returned from all of the member functions here"?

Comment 11

[Daniel Wang]

see http://www.mozilla.org/projects/embedding/embedapiref/embedapi46.html

Comment 12

[Doug Turner (:dougt)]

darin, you should try to intergrate as much of this as you can into your other
nsIFile bug.

Comment 13

[Sören 'Chucker' Kuklau (gone)]

I'm sorry, I don't have time to work on this any longer.

Comment 14

[Doug Turner (:dougt)]

Comment on attachment 110321 [details] [diff] [review]
(Hopefully) addressed biesi's comments

clearing review flags.