-
Notifications
You must be signed in to change notification settings - Fork 153
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.
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
Fixed #665: Replaced var with let or const and added strict mode fs.a… #688
Conversation
Codecov Report
@@ Coverage Diff @@
## master #688 +/- ##
=======================================
Coverage 86.71% 86.71%
=======================================
Files 16 16
Lines 1746 1746
=======================================
Hits 1514 1514
Misses 232 232
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good. What you have is correct, but you can actually change all your let
statements to const
, since they are for variables that won't change their values ever.
Can you do that for me, and push up another commit? When that's done, let me know and I'll re-review this and hopefully we can merge.
tests/spec/fs.access.spec.js
Outdated
@@ -41,8 +43,8 @@ describe('fs.access', function () { | |||
}); | |||
|
|||
it('should return no error if file does exist and mode = F_OK', function (done) { | |||
var fs = util.fs(); | |||
var contents = 'This is a file.'; | |||
let fs = util.fs(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can follow the same pattern as above, and use const
here, since fs
won't change during the scope of this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replaced all let statements with const and pushed another commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect!
Updated the code in filer/tests/spec/fs.access.spec.js to fix issue #665: