Fixed #699 Replacing var with let/const and adding strict mode #706
Conversation
Codecov Report
@@ Coverage Diff @@
## master #706 +/- ##
=======================================
Coverage 86.71% 86.71%
=======================================
Files 16 16
Lines 1746 1746
=======================================
Hits 1514 1514
Misses 232 232
Continue to review full report at Codecov.
|
tests/spec/path-resolution.spec.js
Outdated
| @@ -241,7 +242,7 @@ describe('path resolution', function() { | |||
| }); | |||
|
|
|||
| it('should properly remove trailing slashes with Path.removeTrailing()', function() { | |||
| var Path = Filer.Path; | |||
| let Path = Filer.Path; | |||
There was a problem hiding this comment.
| let Path = Filer.Path; | |
| const Path = Filer.Path; |
Great job with the code, I just think that since path wont be changing later on, you should put it const instead of let.
tests/spec/path-resolution.spec.js
Outdated
| @@ -232,7 +233,7 @@ describe('path resolution', function() { | |||
| }); | |||
|
|
|||
| it('should properly add trailing slashes with Path.addTrailing()', function() { | |||
| var Path = Filer.Path; | |||
| let Path = Filer.Path; | |||
There was a problem hiding this comment.
I think this is the same with the previous one, the path should be a const because It will not be used later on
tests/spec/path-resolution.spec.js
Outdated
|
|
||
| // Support global URL and node's url module | ||
| var URL = global.URL || require('url').URL; | ||
| let URL = global.URL || require('url').URL; |
There was a problem hiding this comment.
This can be const, it won't change.
tests/spec/path-resolution.spec.js
Outdated
|
|
||
| describe('path resolution', function() { | ||
| beforeEach(util.setup); | ||
| afterEach(util.cleanup); | ||
|
|
||
| it('should follow a symbolic link to the root directory', function(done) { | ||
| var fs = util.fs(); | ||
| let fs = util.fs(); |
There was a problem hiding this comment.
const here too, since fs will always be the same for this block.
tests/spec/path-resolution.spec.js
Outdated
| @@ -19,7 +20,7 @@ describe('path resolution', function() { | |||
| if(error) throw error; | |||
|
|
|||
| expect(result['node']).to.exist; | |||
| var _node = result['node']; | |||
| let _node = result['node']; | |||
There was a problem hiding this comment.
Same thing here. In fact, I don't think I see any let below that can't become a const. Do you want to try changing them all, and see what eslint and the tests say?
Upon review and further inspection, there was a realization that there was need for the "let" variable as all variable were not modified. Instead, "const" was used.
Updating for better coding practices.