added few more examples to fs.access. Fixes #17508 #17578
Conversation
nodejs-github-bot
added
doc
|
Hi, @mfaheemakhtar! Welcome and thanks for the pull request! The text explanations have a number of very small issues but rather than enumerate them, I'm wondering if any of the explanatory text you added is necessary. I wonder if it all might be better as code comments in the examples you provide. For example, perhaps the first code sample could go from this: fs.access('./package.json', fs.constants.F_OK, (err) => {
console.log(err ? 'No package.json file!' : 'package.json exists!');
});...to this: // Check if `package.json` exists in the current directory.
fs.access('./package.json', fs.constants.F_OK, (err) => {
console.log(err ? 'No package.json file!' : 'package.json exists!');
});With the comment there, the two preceding sentences aren't needed anymore. The code is instead explained in the comments. What do you think? |
|
Hi @Trott , great idea! I'll improve the pull request. Thanks for the suggestion. UPDATE: I've made few changes. I would love to hear your feedback. |
doc/api/fs.md
Outdated
| `/etc/passwd` can be read and written by the current process. | ||
| argument will be populated. | ||
|
|
||
| The following examples explains the use of different fs.constants. |
There was a problem hiding this comment.
I think we can remove this sentence too. If you do wish to keep it, please change explains to explain and surround fs.constants with `.
There was a problem hiding this comment.
Sure. I've removed the text. Any more feedback?
| ```js | ||
| // Check if we can write to `package.json` file. | ||
| fs.access('./package.json', fs.constants.W_OK, (err) => { | ||
| console.log(err ? 'You cannot write to package.json.' : 'You can write to package.json.'); |
| if(err) { | ||
| if(err.code === 'ENOENT') | ||
| console.error("File does not exist."); | ||
| if(err.code === 'EPERM') |
There was a problem hiding this comment.
Whitespace after the if in if(.
| if(err) | ||
| console.error("Either the file does not exist or it is read-only."); | ||
| else | ||
| console.log("The file exists. You can write to the file."); | ||
| }); |
There was a problem hiding this comment.
This example is pretty redundant as the only difference is checking for the error code. It should be obvious from the example above what to do.
There was a problem hiding this comment.
Thank you, @BridgeAR. I appreciate your feedback.
| }); | ||
| ``` | ||
|
|
||
| ```js |
There was a problem hiding this comment.
Since this is all about examples, we could actually use a single JS block that includes all the different examples. But that might just be my personal preference.
|
@tabulatedreams thanks again for your PR. Great that you are helping out! Just some hints about how to make things easier, I recommend to run |
| ``` | ||
|
|
||
| ```js | ||
| // Check if we can read `package.json` file. |
There was a problem hiding this comment.
Please avoid using informal pronouns like "we" in the docs
There was a problem hiding this comment.
...so maybe Check if package.json file can be read or something similar to that...
|
Ping @mfaheemakhtar, can you address the reviews and update this PR? Also looks like it needs a rebase. Thanks! |
|
Sorry @joyeecheung, I had completely forgotten about this. Unfortunately, I am very busy nowadays and can't review and/or update PR. You can consider this PR as outdated and can be rejected/deleted. Thank you. :) |
|
@mfaheemakhtar Sorry to hear that. I'll close this one out at the moment but if anyone in this thread wants to pursue this feel free to reopen or submit another PR. |
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)