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.

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

Update fs.spec.js to use strict mode and changed var to let #693

Merged
merged 3 commits into from
Feb 12, 2019

Conversation

OnesQuared
Copy link
Contributor

Updated to use strict mode to detect silent errors
changed the var to let in order to be transferable in the future

@codecov-io
Copy link

codecov-io commented Jan 28, 2019

Codecov Report

Merging #693 into master will decrease coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #693      +/-   ##
==========================================
- Coverage   86.71%   86.63%   -0.08%     
==========================================
  Files          16       16              
  Lines        1746     1736      -10     
==========================================
- Hits         1514     1504      -10     
  Misses        232      232
Impacted Files Coverage Δ
src/filesystem/interface.js 93.29% <0%> (-0.39%) ⬇️
src/fs-watcher.js 92.3% <0%> (ø) ⬆️
src/shell/environment.js 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26b47ee...e643b15. Read the comment docs.

@woosle1234
Copy link
Contributor

woosle1234 commented Jan 29, 2019

For the require lines at the top of the file it's better to use const instead of let especially since those are global declarations also shouldn't be modifiable.

Copy link
Contributor

@woosle1234 woosle1234 left a comment

Choose a reason for hiding this comment

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

All the require lines should be const

@@ -1,19 +1,21 @@
var Filer = require('../../src');
var util = require('../lib/test-utils.js');
var expect = require('chai').expect;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should change to const

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

This is really close. I agree with @woosle1234, you can switch to const for all your let's below. Every one of these variables is set, but then never changes; perfect for const.

Can you update that, push again, and let me know when it's done? I'll re-review and hopefully we can merge.

Changed global variables
@OnesQuared
Copy link
Contributor Author

@humphd @woosle1234 The global variables have been changed as per request, please review the code again and let me know if anything else would need to be changed


describe('fs', function() {
beforeEach(util.setup);
afterEach(util.cleanup);

it('is an object', function() {
var fs = util.fs();
let fs = util.fs();
Copy link
Contributor

Choose a reason for hiding this comment

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

const here too, fs never changes.

expect(typeof fs).to.equal('object');
expect(fs).to.be.an.instanceof(Filer.FileSystem);
});

it('should have a root directory', function(done) {
var fs = util.fs();
let fs = util.fs();
Copy link
Contributor

Choose a reason for hiding this comment

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

And here, const, fs never changes.

changed let to const
@humphd humphd merged commit f2201e7 into filerjs:master Feb 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants