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 #662: exchange var for const of require statements in src/filesystem/interface.js #668

Merged
merged 4 commits into from Feb 1, 2019

Conversation

Projects
None yet
4 participants
@trowacat
Copy link
Contributor

trowacat commented Jan 24, 2019

Changed decleration of import files from var to const in interface.js

exchange var for const of require statements
Changed decleration of import files from var to const in interface.js
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 24, 2019

Codecov Report

Merging #668 into master will decrease coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #668      +/-   ##
==========================================
- 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% <100%> (-0.39%) ⬇️
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...8caf907. Read the comment docs.

@andrewkoung

This comment has been minimized.

Copy link
Contributor

andrewkoung commented Jan 24, 2019

I don't disagree with any of the changes made, but there there are still more stuff in the file that could be changed.

@humphd
Copy link
Contributor

humphd left a comment

This is a good start, nice work. I left some things to address. Also, can you also dig into the rest of the file, and also fix those uses of var?

NOTE: you can git add, git commit and git push again on this same branch, and it will add more to this pull request.

var FS_PENDING = Constants.FS_PENDING;
var FS_ERROR = Constants.FS_ERROR;
var FS_NODUPEIDCHECK = Constants.FS_NODUPEIDCHECK;
const Constants = require('../constants.js');

This comment has been minimized.

@humphd

humphd Jan 25, 2019

Contributor

Let's use destructing assignment here, since we're pulling a bunch of stuff off Constants:

const {
  FILE_SYSTEM_NAME,
  FS_FORMAT,
  ...
} = require('../constants.js');
@@ -1,30 +1,30 @@
var { promisify } = require('es6-promisify');

This comment has been minimized.

@humphd

humphd Jan 25, 2019

Contributor

Can you add use strict; to the top of this file please?

trowacat added some commits Jan 25, 2019

@trowacat

This comment has been minimized.

Copy link
Contributor Author

trowacat commented Jan 25, 2019

@humphd Thank you for the recommendations, I've made all the additions you requested. Let me know if there is still more that needs to be done!

@humphd
Copy link
Contributor

humphd left a comment

This was a big file to take on, with lots of changes needed. Nice job. I've left some comments on things you can improve next time, and some things to fix now.

Please update this, and let me know when you've pushed the fixes so I can re-review and hopefully merge.

Show resolved Hide resolved src/filesystem/interface.js Outdated
Show resolved Hide resolved src/filesystem/interface.js Outdated

// node.js supports a calling pattern that leaves off a callback.
function maybeCallback(callback) {
if(typeof callback === 'function') {
if (typeof callback === 'function') {

This comment has been minimized.

@humphd

humphd Jan 31, 2019

Contributor

In general, be careful of this kind of thing (i.e., changing formatting in lines unrelated to your actual changes). We call this a "spurious whitespace change" and open source projects tend to want to avoid it, since it masks the history of this line of code, making it look like it was changed here by you, when actually it was just a formatting fix.

I'll allow it this time, but be aware that it's not generally a good idea. If your editor did it automatically, I'd disable that.

Show resolved Hide resolved src/filesystem/interface.js Outdated
Show resolved Hide resolved src/filesystem/interface.js Outdated
Show resolved Hide resolved src/filesystem/interface.js Outdated
@trowacat

This comment has been minimized.

Copy link
Contributor Author

trowacat commented Feb 1, 2019

@humphd Thank you again for the input! I believe all is in order now.

@humphd

humphd approved these changes Feb 1, 2019

Copy link
Contributor

humphd left a comment

Nice.

@humphd humphd merged commit 3b4bf42 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.