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.

Sign up for GitHub

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

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Feb 12, 2018
tniessen
tniessen approved these changes Feb 12, 2018
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

Thank you!

@BridgeAR BridgeAR added fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 12, 2018
@BridgeAR
Copy link
Member

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

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

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

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

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

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

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 commented Feb 12, 2018

@apapirovski @BridgeAR I updated PR

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

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Still LGTM

bnoordhuis
bnoordhuis reviewed Feb 12, 2018
View reviewed 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

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

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

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

@BridgeAR
Copy link
Member

@daynin this needs a rebase.

@BridgeAR BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 17, 2018
@daynin
Copy link
Contributor Author

daynin commented Feb 17, 2018

@BridgeAR done

@BridgeAR
Copy link
Member

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

mmarchini pushed a commit that referenced this pull request Feb 17, 2018
@daynin
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
Contributor

Landed in 472cde6, thanks! 😄

@mmarchini mmarchini closed this Feb 17, 2018
MylesBorins pushed a commit that referenced this pull request 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 pushed a commit that referenced this pull request 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 pushed a commit that referenced this pull request 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 pushed a commit to MayaLekova/node that referenced this pull request 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 bnoordhuis left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@mmarchini mmarchini mmarchini approved these changes

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@apapirovski apapirovski apapirovski approved these changes

Assignees
No one assigned
Labels
fast-track PRs that do not need to wait for 48 hours to land. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.