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

fixes issue #667 #678

Merged
merged 7 commits into from Feb 2, 2019

Conversation

Projects
None yet
4 participants
@jadach1
Copy link
Contributor

jadach1 commented Jan 26, 2019

Update code in tests/some/fs.write.spec.js to use const/let AND strict mode

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 26, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #678      +/-   ##
==========================================
- 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...dcb3333. Read the comment docs.

@vlogozzo
Copy link
Contributor

vlogozzo left a comment

many changes can be const instead of let, review the inline comments


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

it('should be a function', function() {
var fs = util.fs();
let fs = util.fs();

This comment has been minimized.

@vlogozzo

vlogozzo Jan 30, 2019

Contributor

fs can be const as it isnt changed in this scope

Suggested change
let fs = util.fs();
const fs = util.fs();
var fs = util.fs();
var fn = () => fs.writeFile(undefined, 'data');
let fs = util.fs();
let fn = () => fs.writeFile(undefined, 'data');

This comment has been minimized.

@vlogozzo

vlogozzo Jan 30, 2019

Contributor

fn can be const as well

Suggested change
let fn = () => fs.writeFile(undefined, 'data');
const fn = () => fs.writeFile(undefined, 'data');
var fs = util.fs();
var buffer = Buffer.from([1, 2, 3, 4, 5, 6, 7, 8]);
let fs = util.fs();
let buffer = Buffer.from([1, 2, 3, 4, 5, 6, 7, 8]);

This comment has been minimized.

@vlogozzo

vlogozzo Jan 30, 2019

Contributor

buffer is just passed into a function it can be const

Suggested change
let buffer = Buffer.from([1, 2, 3, 4, 5, 6, 7, 8]);
constt buffer = Buffer.from([1, 2, 3, 4, 5, 6, 7, 8]);
@humphd
Copy link
Contributor

humphd left a comment

I agree with the other review comments here, please make the updates.

Also, please back-out your changes to package-lock.json:

git checkout master package-lock-json

Then you can commit and push that to this branch, and it won't include this file. It's not related to your changes, so doesn't belong in this PR.

expect(fs.write).to.be.a('function');
});

it('should error if file path is undefined', function() {
var fs = util.fs();
var fn = () => fs.writeFile(undefined, 'data');
let fs = util.fs();

This comment has been minimized.

@humphd

humphd Feb 1, 2019

Contributor

const here, since fs won't change

expect(fn).to.throw();
});

it('should write data to a file', function(done) {
var fs = util.fs();
var buffer = Buffer.from([1, 2, 3, 4, 5, 6, 7, 8]);
let fs = util.fs();

This comment has been minimized.

@humphd

humphd Feb 1, 2019

Contributor

const here too

var fs = util.fs();
var buffer = Buffer.from([1, 2, 3, 4, 5, 6, 7, 8]);
var _result = 0;
let fs = util.fs();

This comment has been minimized.

@humphd

humphd Feb 1, 2019

Contributor

And here, const

@humphd
Copy link
Contributor

humphd left a comment

A couple of things you can change to const, and this is ready to go.

@humphd

humphd approved these changes Feb 2, 2019

Copy link
Contributor

humphd left a comment

Looks great, thanks.

@humphd humphd merged commit da1aad5 into filerjs:master Feb 2, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.