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

fs.copyFile.spec.js: changed var to let or const and added "use strict" #720

Closed
wants to merge 13 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@a-deeb
Copy link

a-deeb commented Jan 30, 2019

fs.copyFile.spec.js :

Added Line: use strict;

changed vars to let or const

a-deeb added some commits Jan 30, 2019

Merge pull request #1 from a-deeb/a-deeb-patch-1
Update fs.copyFile.spec.js
Merge pull request #2 from a-deeb/a-deeb-patch-2
Update fs.copyFile.spec.js
@cmchumak

This comment has been minimized.

Copy link
Contributor

cmchumak commented Jan 30, 2019

use strict; should be in quotations and placed at the top of the file.

a-deeb added some commits Jan 30, 2019

Merge pull request #3 from a-deeb/a-deeb-patch-3
Update fs.copyFile.spec.js
Merge pull request #4 from a-deeb/a-deeb-patch-4
Update fs.copyFile.spec.js
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 30, 2019

Codecov Report

Merging #720 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #720   +/-   ##
=======================================
  Coverage   86.71%   86.71%           
=======================================
  Files          16       16           
  Lines        1746     1746           
=======================================
  Hits         1514     1514           
  Misses        232      232

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...706fdd0. Read the comment docs.

@humphd
Copy link
Contributor

humphd left a comment

This is coming along well. I've left a few comments about places you can use const vs. let. Also, a few more things:

  • For some reason, this diff shows you touching every line in the file. That says to me that your editor has maybe reformatted everything automatically? You want to be very careful about this. When you work on an open source project, you only want to change the lines of code associated with the bug you're fixing. Otherwise, you overwrite the git history of the lines (i.e., it looks like you wrote every line in this file, where you really just edited some of them). I'll allow it here, but you can't do this going forward in other people's projects. Check your editor and figure out what happened, so you can disable it. Or is this due to line ending changes from Unix to Windows?
  • Next, you need to revert your changes to tests/spec/fs.access.spec.js. To do this, git checkout master tests/spec/fs.access.spec.js and then commit that version and push again.
Show resolved Hide resolved tests/spec/fs.copyFile.spec.js Outdated
Show resolved Hide resolved tests/spec/fs.copyFile.spec.js Outdated
Show resolved Hide resolved tests/spec/fs.copyFile.spec.js Outdated
afterEach(util.cleanup);

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

This comment has been minimized.

@humphd

humphd Feb 5, 2019

Contributor

Remove comment, change let to const, since fs doesn't change in this scope.

Show resolved Hide resolved tests/spec/fs.copyFile.spec.js Outdated
});

it('should copy file successfully', function(done) {
let fs = util.fs(); //changed var to let

This comment has been minimized.

@humphd

humphd Feb 5, 2019

Contributor

Remove comment, change let to const, since fs doesn't change in this scope.

});

it('should return an error if flag=COPYFILE_EXCL and the destination file exists', function (done) {
let fs = util.fs(); //changed var to let

This comment has been minimized.

@humphd

humphd Feb 5, 2019

Contributor

Remove comment, change let to const, since fs doesn't change in this scope.

@humphd

This comment has been minimized.

Copy link
Contributor

humphd commented Feb 12, 2019

This is still not right. If you want to redo it and open a new PR, go ahead. But I'm going to close at this point, since it's got too many changes included.

@humphd humphd closed this Feb 12, 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.