Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fs: make F_OK/R_OK/W_OK/X_OK not writable #507

Closed
wants to merge 1 commit into from
Closed

fs: make F_OK/R_OK/W_OK/X_OK not writable #507

wants to merge 1 commit into from

Conversation

JacksonTian
Copy link
Contributor

change the fs.F_OK/R_OK/W_OK/X_OK out of fs.js will lead unexpect
exception. make these properties readonly.

@rvagg
Copy link
Member

rvagg commented Jan 19, 2015

a description would be nice with pull requests to help justify the change

also, it looks like this might break because F_OK is used elsewhere in fs.js

@JacksonTian JacksonTian changed the title fs: disallow overwrite F_OK/R_OK/W_OK/X_OK fs: make F_OK/R_OK/W_OK/X_OK not writable Jan 19, 2015
@JacksonTian
Copy link
Contributor Author

Updated. @rvagg .

@rvagg
Copy link
Member

rvagg commented Jan 19, 2015

thanks, will leave this open for comment for now

@cjihrig
Copy link
Contributor

cjihrig commented Jan 19, 2015

Why not apply this to any of the other constants in fs.js? And, speaking of constants, could we use some new ES6 goodness to define constants?

@JacksonTian
Copy link
Contributor Author

Other constants in fs.js were not been exported. @cjihrig

@cjihrig cjihrig mentioned this pull request Jan 20, 2015
Closed
@fengmk2
Copy link
Contributor

fengmk2 commented Jan 20, 2015

Use const instead of var will be better.

@JacksonTian
Copy link
Contributor Author

const can make a variable to a constant, but can't make the property be read only.

bnoordhuis
bnoordhuis reviewed Jan 20, 2015
View reviewed changes
lib/fs.js
fs.R_OK = R_OK;
fs.W_OK = W_OK;
fs.X_OK = X_OK;
// don't allow mode to accidentally be overwritten.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: please use proper capitalization in comments.

@bnoordhuis
Copy link
Member

I agree with @cjihrig that it's somewhat inconsistent to apply it only to the fs.access() flags.

@JacksonTian
fs: make F_OK/R_OK/W_OK/X_OK not writable.
c124fee 
change the fs.F_OK/R_OK/W_OK/X_OK out of fs.js will lead unexpect
exception. make these properties readonly.
@JacksonTian
Copy link
Contributor Author

@bnoordhuis Updated by your comment. Thanks.

@bnoordhuis
Copy link
Member

LGTM but I defer to @cjihrig.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 21, 2015

I'll land this today.

cjihrig pushed a commit that referenced this pull request Jan 21, 2015
@JacksonTian @cjihrig
fs: make fs.access() flags read only
8b98096 
These flags were defined as constants, but could be
overwritten when exported from fs. This commit exports
the flags as read only properties of fs.

PR-URL: #507
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@cjihrig
Copy link
Contributor

cjihrig commented Jan 21, 2015

Thanks! Landed with a slightly reworded commit message in 8b98096.

@cjihrig cjihrig closed this Jan 21, 2015
@ChALkeR ChALkeR added the fs Issues and PRs related to the fs subsystem / file system. label Feb 16, 2016
@aliscco aliscco mentioned this pull request Apr 28, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@JacksonTian @rvagg @cjihrig @fengmk2 @bnoordhuis @ChALkeR