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

benchmark,path: remove unused variables #15789

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@Aladdin-ADD
Contributor

Aladdin-ADD commented Oct 5, 2017

some are detected by eslint.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
@bnoordhuis

I appreciate you taking the time to put together a PR but none of these changes look even remotely correct.

What exactly did you do and did you run make test?

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Oct 5, 2017

Contributor

The changes to lib/path.js appear legit, but IMHO it would be better to have them in a separate PR.

Contributor

mscdex commented Oct 5, 2017

The changes to lib/path.js appear legit, but IMHO it would be better to have them in a separate PR.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Oct 5, 2017

Contributor

Also, changes to dependencies should be made in those projects' respective upstream repositories instead of node's repository.

The only remaining change is the change to benchmark/fs/read-stream-throughput.js which seems legit also. In that case, the commit message for this PR should be changed to target the benchmark subsystem (when all of the other changes are removed).

Contributor

mscdex commented Oct 5, 2017

Also, changes to dependencies should be made in those projects' respective upstream repositories instead of node's repository.

The only remaining change is the change to benchmark/fs/read-stream-throughput.js which seems legit also. In that case, the commit message for this PR should be changed to target the benchmark subsystem (when all of the other changes are removed).

@Aladdin-ADD

This comment has been minimized.

Show comment
Hide comment
@Aladdin-ADD

Aladdin-ADD Oct 6, 2017

Contributor

sorry for the delay. I reset the changes in deps/*. 😃

my local testing failing somehow, but should not be related to the changes -- it was the same when reverting all changes.

image

friendly ping @mscdex @bnoordhuis

Contributor

Aladdin-ADD commented Oct 6, 2017

sorry for the delay. I reset the changes in deps/*. 😃

my local testing failing somehow, but should not be related to the changes -- it was the same when reverting all changes.

image

friendly ping @mscdex @bnoordhuis

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Oct 6, 2017

Contributor

I think the commit message should be changed if we're going to keep these two changes together. Perhaps something like "benchmark,path: remove unused variables" because our linter currently does not fail without the changes, so the current message is misleading.

Contributor

mscdex commented Oct 6, 2017

I think the commit message should be changed if we're going to keep these two changes together. Perhaps something like "benchmark,path: remove unused variables" because our linter currently does not fail without the changes, so the current message is misleading.

@Aladdin-ADD Aladdin-ADD changed the title from chore: fix some linting errors. to benchmark,path: remove unused variables Oct 6, 2017

@lpinca

lpinca approved these changes Oct 9, 2017

@lpinca

This comment has been minimized.

Show comment
Hide comment
@Aladdin-ADD

This comment has been minimized.

Show comment
Hide comment
@Aladdin-ADD

Aladdin-ADD Oct 14, 2017

Contributor

friendly ping @bnoordhuis (。・∀・)ノ

Contributor

Aladdin-ADD commented Oct 14, 2017

friendly ping @bnoordhuis (。・∀・)ノ

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung
Member

joyeecheung commented Oct 15, 2017

Ping @bnoordhuis PTAL

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Oct 19, 2017

Member

Ping @bnoordhuis your comment seems to be addressed.

Member

BridgeAR commented Oct 19, 2017

Ping @bnoordhuis your comment seems to be addressed.

@apapirovski

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Oct 26, 2017

Member

Hi @bnoordhuis — it seems your feedback here might've been addressed. Would you mind giving it a look? Thanks!

Member

apapirovski commented Oct 26, 2017

Hi @bnoordhuis — it seems your feedback here might've been addressed. Would you mind giving it a look? Thanks!

The comment was addressed and seems resolved. PTAL

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
Member

BridgeAR commented Nov 22, 2017

jasnell added a commit that referenced this pull request Nov 22, 2017

benchmark,path: remove unused variables
PR-URL: #15789
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Nov 22, 2017

Member

Landed in fbb9eef

Member

jasnell commented Nov 22, 2017

Landed in fbb9eef

@jasnell jasnell closed this Nov 22, 2017

@addaleax addaleax removed the author ready label Nov 28, 2017

MylesBorins added a commit that referenced this pull request Dec 12, 2017

benchmark,path: remove unused variables
PR-URL: #15789
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

MylesBorins added a commit that referenced this pull request Dec 12, 2017

benchmark,path: remove unused variables
PR-URL: #15789
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

@MylesBorins MylesBorins referenced this pull request Dec 12, 2017

Merged

v9.3.0 proposal #17631

gibfahn added a commit that referenced this pull request Dec 19, 2017

benchmark,path: remove unused variables
PR-URL: #15789
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

@gibfahn gibfahn referenced this pull request Dec 20, 2017

Closed

v8.9.4 proposal #17772

gibfahn added a commit that referenced this pull request Dec 20, 2017

benchmark,path: remove unused variables
PR-URL: #15789
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

@gibfahn gibfahn referenced this pull request Dec 20, 2017

Merged

v8.9.4 proposal #17774

MylesBorins added a commit that referenced this pull request Dec 20, 2017

benchmark,path: remove unused variables
PR-URL: #15789
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

@MylesBorins MylesBorins referenced this pull request Dec 20, 2017

Merged

v6.12.3 proposal #17776

msoechting added a commit to hpicgs/node that referenced this pull request Feb 5, 2018

benchmark,path: remove unused variables
PR-URL: nodejs#15789
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

benchmark,path: remove unused variables
PR-URL: nodejs#15789
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment