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

Issue 710 fixed #729

Closed
wants to merge 17 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@abdiG
Copy link

abdiG commented Jan 30, 2019

In file filer/tests/spec/fs.exists.spec.js i changed var to let and added 'use strick'

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 30, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #729      +/-   ##
=========================================
- Coverage   86.71%   86.6%   -0.12%     
=========================================
  Files          16      16              
  Lines        1746    1739       -7     
=========================================
- Hits         1514    1506       -8     
- Misses        232     233       +1
Impacted Files Coverage Δ
src/filesystem/interface.js 93.29% <0%> (-0.39%) ⬇️
src/filesystem/implementation.js 83.68% <0%> (-0.05%) ⬇️
src/stats.js 100% <0%> (ø) ⬆️
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...6f376ae. Read the comment docs.

@kwchan19

This comment has been minimized.

Copy link
Contributor

kwchan19 commented Jan 31, 2019

I'm seeing that you used 'let' for all of the variables. It would be better to use 'const', as the variables do not change values.

@humphd
Copy link
Contributor

humphd left a comment

This is coming along nicely. A few things:

  1. I agree with your reviewer, you can change to const vs. let since your variables aren't changing.
  2. You have extra file changes in here that aren't related to your change (i.e., dist/* and package-lock.json). You can revert these using git checkout master dist and git checkout master package-lock.json. Please do that and then git add, git commit, and git push to this branch again.

Let me know when these changes are updated and I'll re-review.


describe('fs.exists', 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.

@humphd

humphd Feb 5, 2019

Contributor

This can get changed to const since fs won't change in the current scope.

expect(typeof fs.exists).to.equal('function');
});

it('should return false if path does not exist', function(done) {
var fs = util.fs();
let fs = util.fs();

This comment has been minimized.

@humphd

humphd Feb 5, 2019

Contributor

This can get changed to const since fs won't change in the current scope.

@@ -20,7 +21,7 @@ describe('fs.exists', function() {
});

it('should return true if path exists', function(done) {
var fs = util.fs();
let fs = util.fs();

This comment has been minimized.

@humphd

humphd Feb 5, 2019

Contributor

This can get changed to const since fs won't change in the current scope.

@@ -37,7 +38,7 @@ describe('fs.exists', function() {
});

it('should follow symbolic links and return true for the resulting path', function(done) {
var fs = util.fs();
let fs = util.fs();

This comment has been minimized.

@humphd

humphd Feb 5, 2019

Contributor

This can get changed to const since fs won't change in the current scope.

@@ -58,7 +59,7 @@ describe('fs.exists', function() {
});

it('should follow symbolic links and return false if for the resulting path does not exist', function(done) {
var fs = util.fs();
let fs = util.fs();

This comment has been minimized.

@humphd

humphd Feb 5, 2019

Contributor

This can get changed to const since fs won't change in the current scope.

@humphd

This comment has been minimized.

Copy link
Contributor

humphd commented Feb 8, 2019

@abdiG how is this coming?

Abdirahman Guled added some commits Feb 8, 2019

@humphd
Copy link
Contributor

humphd left a comment

It looks like this removed the dist/ files completely, which we don't want. Please do this:

git checkout master dist/

Then you can commit those and push again.

@humphd

This comment has been minimized.

Copy link
Contributor

humphd commented Feb 12, 2019

@abdiG how is this coming?

@abdiG

This comment has been minimized.

Copy link
Author

abdiG commented Feb 13, 2019

@humphd I have made all the changes you asked. And sorry it took a while.

@humphd
Copy link
Contributor

humphd left a comment

OK, this is getting closer. A few more issues.

It looks like you've run npm in the tests/spec directory, which as created two new files tests/spec/package-lock.json and tests/spec/package.json. Both need to be removed:

git rm -f tests/spec/package*

Then commit and push that.

abdiG added some commits Feb 14, 2019

@humphd

This comment has been minimized.

Copy link
Contributor

humphd commented Feb 15, 2019

Fix and merged in 3d10d64

@humphd humphd closed this Feb 15, 2019

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.