Return value of fs.unlinkSync not documented

[binarykitchen]

Somehow http://nodejs.org/api/fs.html#fs_fs_unlinksync_path does not tell what it's return value is. I'd like to know it. Would be good to add this on that page.

Thanks!

[cjihrig]

I don't think it returns anything (undefined). I personally think it would be useful if the return value of every method was documented, even when it's undefined, I'm not sure if everyone else would like that. Also, are you volunteering to do the potential docs work?

[binarykitchen]

Well IMO those developers who have been working on fs should document it. It's a bad idea if someone else, who never dealt with fs, like me, is documenting it.

All I want to know what happens when unlinkSync() fails to perform due to invalid permissions? Will it throw an exception or just return false?

[cjihrig]

Well IMO those developers who have been working on fs should document it. It's a bad idea if someone else, who never dealt with fs, like me, is documenting it.

You're right. Unfortunately, the documentation is what it is, and help is welcome.

All I want to know what happens when unlinkSync() fails to perform due to invalid permissions? Will it throw an exception or just return false?

An exception is thrown. In most cases, if an error would be passed to an asynchronous callback, it translates into a thrown exception in synchronous code.

[misterdjules]

@binarykitchen @cjihrig We could make the introduction to http://nodejs.org/api/fs.html clearer by mentioning that the return value of synchronous versions is undefined, and that the way this API communicates errors is via exceptions that must be caught to be handled.

What do you think?

[binarykitchen]

@misterdjules Perfect. But make sure to highlight it.

@cjihrig How about adding a policy enforcing every developer to update the documentation whenever they modify the API?

[misterdjules]

Ok, reopening this issue and added the easy label so that someone can pick it up as a first contribution.

[cjihrig]

@binarykitchen such a policy exists. It probably always existed. Sometimes things just slip through the cracks. I'm not trying to make any excuses for the docs though.

[tyleranton]

I'd like to take this if it's still up for grabs. It'd be my first contribution.

[misterdjules]

@tyleranton That's great!

Please not however that I do not agree anymore with what I said in my previous comment :)

Basically, I think that instead of mentioning in the introduction that "all functions for which the returned value is not mentioned return undefined", I think we sould mention the return value in each function's documentation section.

The reason for that is that users can jump to specific functions in the documentation without reading the introduction (it's an API documentation, not a tutorial), and thus they would miss this information.

So my suggestion would be, depending on the time you have, to either just fix the documentation for fs.unlinkSync by adding that it returns undefined, or if you have more time to fix that for every synchronous function of the fs module that returns undefined.

Does that make sense? @joyent/node-coreteam What do you think?

If you need help with any of this, I can help you in #libuv on Freenode (my nickname is jgi), or here on GitHub.

Thank you!

[tyleranton]

@misterdjules I'd be more than glad to update each function's doc. I'm assuming the change needs to made in both the nodejs website AND the fs.markdown in nodejs?

As for verbiage, is "Returns undefined." sufficient?

Thanks!

[misterdjules]

@tyleranton Excellent, thank you! The change actually needs to be made only in nodejs. For each new release, the docs are generated from node's repository and deployed to the nodejs.org website.

To test your changes to the documentation, you can run make doc and open out/doc/api/index.html.

"Returns undefined." is good, but we will probably want to format "undefined" as code with undefined.

[tyleranton]

@misterdjules Great! I'm already on it. This change needs to be made for every single synchronous form that doesn't already explicitly state what it returns, correct?

[misterdjules]

@tyleranton Right, and in addition to that we would want to check in the code that these functions actually return undefined. That's not always obvious, especially if you're new to the code base, but I can help you with that too.

Another thing I forgot to mention is that we'll want to make these changes to the v0.10 branch first, and then when it's done we'll merge these changes to v0.12 and master. So make sure you checkout the v0.10 branch before making the changes.

I won't be online tonight, but I'll be online tomorrow and Sunday so I'll be able to help you with this over the week-end if you'd like.

Thanks again!

[tyleranton]

@misterdjules Great, thanks! I've made the changes and tested them in markdown preview and the generated site.

I'll start checking the changes against the code and let you know if I get stuck tomorrow.

Excited to make my first contribution!

[tyleranton]

@misterdjules In a few instances, the async and sync functions are grouped together with a single description, like so:

fs.utimes(path, atime, mtime, callback)

fs.utimesSync(path, atime, mtime)

Change file timestamps of the file referenced by the supplied path.

Should this be changed to:

fs.utimes(path, atime, mtime, callback)

Change file timestamps of the file referenced by the supplied path.

fs.utimesSync(path, atime, mtime)

Synchronous version of fs.utimes. Returns undefined.

[misterdjules]

@tyleranton Yes, I think it should be changed like you did.

[tyleranton]

@misterdjules Great, I've made the changes. I've compared it to the code, and it all seems correct to me, but it may be a safe bet to double check behind me.

I'm going to go ahead and double check all of my changes again and with your go ahead, I'll commit and make a PR.

Thanks!

[tyleranton]

@misterdjules Pushed the changes to my branch. I'm ready to make a pull request.

[misterdjules]

@tyleranton Your changes look good, but it seems that they include content from the v0.12 branch.

Is it possible that you had the v0.12 branch loaded in your editor, then checked out the v0.10 branch and saved the doc/api/fs.markdown file from v0.12 that was loaded in memory in your local v0.10 branch?

[tyleranton]

@misterdjules Ah, that's probably what happened. I've copied the one from v0.10 and made the changes again. Do I need to provide a different commit message?

[misterdjules]

@tyleranton The commit message looked good 👍

[tyleranton]

@misterdjules Great, pushed the changes. Ready to make a PR.

[misterdjules]

@tyleranton Great! Before creating the pull request, you will need to squash the two commits that are in your branch into one commit with git rebase -i HEAD~2. Then create the pull request against the v0.10 branch in joyent/node and I'll review it.

If you need help with any of this I'm currently in #libuv on Freenode as jgi.

[misterdjules]

Fixed by 51fe319, thank you @binarykitchen, @tyleranton and @cjihrig!