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#719: Updated var to const/let #731

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@ael-mas
Copy link
Contributor

ael-mas commented Jan 31, 2019

Updated var declarations to use const or let instead.
Added strict mode to file.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 31, 2019

Codecov Report

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

Impacted file tree graph

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

@humphd
Copy link
Contributor

humphd left a comment

This looks great. I've left some feedback about places to use const instead of let. Can you do those changes, push again on this branch, and let me know when it's updated? Then I can re-review.

Show resolved Hide resolved tests/spec/fs.read.spec.js
var rbuffer = Buffer.alloc(wbuffer.length);
let fs = util.fs();
let wbuffer = Buffer.from([1, 2, 3, 4, 5, 6, 7, 8]);
let rbuffer = Buffer.alloc(wbuffer.length);

This comment has been minimized.

@humphd

humphd Feb 5, 2019

Contributor

Same for all of these 3 variables: use const since the variable won't change again in this scope.

let fs = util.fs();
let wbuffer = Buffer.from([1, 2, 3, 4, 5, 6, 7, 8]);
let rbuffer = Buffer.alloc(wbuffer.length);
let _result = 0;

This comment has been minimized.

@humphd

humphd Feb 5, 2019

Contributor

Same with fs, wbuffer, rbuffer, they can all be const. You're right to use let for _result though.

var buf2 = Buffer.alloc(20);
let fs = util.fs();
let buf = Buffer.alloc(20);
let buf2 = Buffer.alloc(20);

This comment has been minimized.

@humphd

humphd Feb 5, 2019

Contributor

const for all of these 3

var fd = 0;
var rbuffer = Buffer.alloc(8);
let fd = 0;
let rbuffer = Buffer.alloc(8);

This comment has been minimized.

@humphd

humphd Feb 5, 2019

Contributor

const for all three of these.

@humphd

This comment has been minimized.

Copy link
Contributor

humphd commented Feb 8, 2019

@ael-mas how is this coming? Any update?

@humphd

This comment has been minimized.

Copy link
Contributor

humphd commented Feb 12, 2019

@ael-mas did you want this PR closed in favour of the new one you opened?

@ael-mas

This comment has been minimized.

Copy link
Contributor Author

ael-mas commented Feb 12, 2019

The new one should have the correct variable declarations using const instead of let. Apologies if I didn't modify my original changes properly on github.

@humphd

This comment has been minimized.

Copy link
Contributor

humphd commented Feb 12, 2019

OK, no problem. Closing in favour of #741. In future, you can just make more commits and push to your same branch, and it will add to/update an existing PR vs. making a new one.

@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.