Conversation
src/chmod.js
Outdated
| OWNER_READ: base.READ << 6, | ||
|
|
||
| // Literal octal numbers are apparently not allowed in "strict" javascript. Using parseInt is | ||
| // the preferred way, else a jshint warning is thrown. |
Contributor
There was a problem hiding this comment.
Two things:
- It might be good to note that they've been added back in ES6, for future reference.
- Maybe change the reference to jshint to eslint?
Member
Author
There was a problem hiding this comment.
I'll fix the reference to jshint. The part about octals was around previously, so I think we can just fix that in a new PR
Contributor
|
I went through and added some comments, one thing in perticular that I think we should change is this: // This is OK without curly braces, because it's just a guard statement:
if (whatever) return doFoo();
// This is NOT OK, because it might be confusing in the future:
if (whatever)
return doFoo(); // Note the newline.
// This is OK, because there is braces:
if (whatever) {
return doFoo();
} |
Member
Author
|
@ariporad do you know the eslint rule for that? |
Member
Author
|
@ariporad just updated. Your comments should be addressed. I left the section about octal numbers, since it isn't really relevant to updating linting rules. |
Contributor
|
LGTM! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #317
This switches us to using eslint instead of jshint, based off the airbnb style guide for es5.
Note: Most of the changes were created by
eslint --fix.I disabled linting rules whenever I encountered too many places where they would need fixing. Here are the rules I disabled (which we can go back and re-enable at our convenience):
Any other refactors that I did manually should be mostly semantically equivalent to the way the code was before. Feel free to take your time reviewing, since there's the potential for things to change.
for (key in obj)loop was changed toObject.keys(obj).forEach(function (key) {(which I believe is slightly different semantically, but shouldn't have an effect on the intended usage).no-else-returnerrors, but ultimately disabled the rule.src/uniq.jsto be an else-if ladder for clarity.consistent-returnerrors, but ultimately disabled the rule (sincecommon.error()actually does implicitly return a value).\@ingenerate-docs.js).defaultcase to a switch statement inchmod()because it didn't have one.Make sure there's no semantic changes in the test files (I don't remember doing any intentionally).