★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/18739Nouvelle recherche | Portail wanayoo
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

Jump to bottom

fs: remove useless comments which duplicate names of variables #18739

Closed
wants to merge 1 commit into from
Closed

fs: remove useless comments which duplicate names of variables #18739

wants to merge 1 commit into from

Conversation

daynin
Copy link
Contributor

@daynin daynin commented Feb 12, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

fs

tniessen
tniessen approved these changes Feb 12, 2018
View changes
Copy link
Member

@tniessen tniessen left a comment

Thank you!

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Feb 12, 2018

I will not run a CI in this case because the change is obvious.

apapirovski
apapirovski approved these changes Feb 12, 2018
View changes
Copy link
Member

@apapirovski apapirovski left a comment

I'm fine with this landing as is but consider my comment.

lib/fs.js Outdated
@@ -2145,7 +2145,7 @@ fs.realpath = function realpath(p, options, callback) {
// dev/ino always return 0 on windows, so skip the check.
let id;
if (!isWindows) {
var dev = statValues[0/*ino*/].toString(32);
var dev = statValues[0/*dev*/].toString(32);
Copy link
Member

@apapirovski apapirovski Feb 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is fine but what about just removing both comments? They are not particularly useful as the variable names already make this obvious. That same change could then be made in another spot in this file where these unnecessary comments exist.

Copy link
Contributor Author

@daynin daynin Feb 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apapirovski you're right! Should I fix it right here or I can fix in a separate pull request?

Copy link
Member

@apapirovski apapirovski Feb 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's being fixed, I would prefer it in this PR to avoid unnecessary commits. Other collaborators might have a different opinion.

@daynin daynin changed the title fs: fix a typo in a comment fs: remove useless comments which duplicate names of variables Feb 12, 2018
@daynin
Copy link
Contributor Author

@daynin daynin commented Feb 12, 2018

@apapirovski @BridgeAR I updated PR

BridgeAR
BridgeAR approved these changes Feb 12, 2018
View changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Still LGTM

lpinca
lpinca approved these changes Feb 12, 2018
View changes
bnoordhuis
bnoordhuis reviewed Feb 12, 2018
View changes
lib/fs.js Outdated
@@ -2752,7 +2752,7 @@ async function readFileHandle(filehandle, options) {

let size;
if ((statFields[1/*mode*/] & S_IFMT) === S_IFREG) {
size = statFields[8/*size*/];
size = statFields[8];
Copy link
Member

@bnoordhuis bnoordhuis Feb 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

statFields should probably be renamed to statValues so that grepping for statValues[8] finds all instances.

Copy link
Contributor Author

@daynin daynin Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis done

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Feb 16, 2018

CI-Light https://ci.nodejs.org/job/node-test-commit-light/273/

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Feb 17, 2018

@daynin this needs a rebase.

@daynin
Copy link
Contributor Author

@daynin daynin commented Feb 17, 2018

@BridgeAR done

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Feb 17, 2018

Light-CI https://ci.nodejs.org/job/node-test-commit-light/292/

mmarchini pushed a commit that referenced this issue Feb 17, 2018
@daynin Matheus Marchini
fs: remove useless comments which duplicate names of variables
472cde6 
PR-URL: #18739
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
@mmarchini
Copy link
Member

@mmarchini mmarchini commented Feb 17, 2018

Landed in 472cde6, thanks! 😄

@mmarchini mmarchini closed this Feb 17, 2018
MylesBorins added a commit that referenced this issue Feb 21, 2018
@daynin @MylesBorins
fs: remove useless comments which duplicate names of variables
bac9487 
PR-URL: #18739
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
Merged
MylesBorins added a commit that referenced this issue Feb 21, 2018
@daynin @MylesBorins
fs: remove useless comments which duplicate names of variables
4601bfd 
PR-URL: #18739
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
MylesBorins added a commit that referenced this issue Feb 21, 2018
@daynin @MylesBorins
fs: remove useless comments which duplicate names of variables
0568f75 
PR-URL: #18739
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
MayaLekova added a commit to MayaLekova/node that referenced this issue May 8, 2018
@daynin @MayaLekova
fs: remove useless comments which duplicate names of variables
881a9d6 
PR-URL: nodejs#18739
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis

@apapirovski apapirovski

@jasnell jasnell

@lpinca lpinca

@cjihrig cjihrig

@tniessen tniessen

@mmarchini mmarchini

@BridgeAR BridgeAR

Assignees
No one assigned
Labels
fast-track fs
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet