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

Fix ssue #692: Update code in filer/tests/spec/time-flags.spec.js #696

Merged
merged 6 commits into from Feb 1, 2019

Conversation

Projects
None yet
5 participants
@Violet-XiaoWeiHuang
Copy link
Contributor

Violet-XiaoWeiHuang commented Jan 29, 2019

"var" was replaced by "let", as the passed variable may be modified later. In this case, I avoid using "const".

@Violet-XiaoWeiHuang Violet-XiaoWeiHuang changed the title Issue 692 Fix ssue #692: Update code in filer/tests/spec/time-flags.spec.js to use "const"/"let" instead of "var" Jan 29, 2019

@Violet-XiaoWeiHuang Violet-XiaoWeiHuang changed the title Fix ssue #692: Update code in filer/tests/spec/time-flags.spec.js to use "const"/"let" instead of "var" Fix ssue #692: Update code in filer/tests/spec/time-flags.spec.js Jan 29, 2019

@rscotchmer

This comment has been minimized.

Copy link
Contributor

rscotchmer commented Jan 29, 2019

Hi! It looks like you're off to a great start, but I think there are some changes to be made yet. On line 13, you've got a comma instead of a semi-colon, which I think is why your Travis CI build is failing -- I copied your code into my local version of filer, and was able to run the tests perfectly after I fixed that.

You're also missing 'use strict' at the top of your file, which you need to use so the file runs in strict mode. I think you could also change the 3 "lets" on lines 3, 4, and 5 to "const"; the values there should never change, so it's safe to use const.

@Violet-XiaoWeiHuang

This comment has been minimized.

Copy link
Contributor Author

Violet-XiaoWeiHuang commented Jan 29, 2019

Thank you for your comment. It's strange I tried locally, all tests passed.
And in line 13 you mentions the comma should be correct as name is one of the data member. @rscotchmer

@rscotchmer

This comment has been minimized.

Copy link
Contributor

rscotchmer commented Jan 29, 2019

I feel you -- I had all tests passing locally, and then when I put up the pull request it decided to fail a few -- the joys of programming, eh? I'm glad all the tests are passing for you now, though!

You're definitely right about that comma! My bad, I should've taken a closer look at that one.
@Violet-XiaoWeiHuang

@ithompson4

This comment has been minimized.

Copy link
Contributor

ithompson4 commented Jan 29, 2019

Hi, I think you are still missing 'use strict' at the top of your file.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 29, 2019

Codecov Report

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

Impacted file tree graph

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

@Violet-XiaoWeiHuang

This comment has been minimized.

Copy link
Contributor Author

Violet-XiaoWeiHuang commented Jan 29, 2019

Thank you @ithompson4.

@humphd
Copy link
Contributor

humphd left a comment

This is on the right track. Two things to do in order to finish it:

  1. Can you revert your changes to package-lock.json? It's unrelated to your other changes, and we tend to make each PR/bug be about one fix. You can do that with git checkout master package-lock.json

  2. You can replace all your lets to const below

After you do that and push a new commit, let me know and I'll re-review.


describe('node times (atime, mtime, ctime) with mount flags', function() {

var dirname = '/dir';
var filename = '/dir/file';
let dirname = '/dir';

This comment has been minimized.

@humphd

humphd Jan 31, 2019

Contributor

All these let keywords can be switched to const, since the variables aren't going to change value.

@humphd

humphd approved these changes Feb 1, 2019

Copy link
Contributor

humphd left a comment

Looks good.

@humphd humphd merged commit 05057c4 into filerjs:master Feb 1, 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.