benchmark,path: remove unused variables#15789
benchmark,path: remove unused variables#15789aladdin-add wants to merge 1 commit intonodejs:masterfrom
Conversation
bnoordhuis
left a comment
There was a problem hiding this comment.
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?
|
The changes to lib/path.js appear legit, but IMHO it would be better to have them in a separate PR. |
|
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). |
3c93b49 to
20dd55f
Compare
|
sorry for the delay. I reset the changes in my local testing failing somehow, but should not be related to the changes -- it was the same when reverting all changes.
friendly ping @mscdex @bnoordhuis |
|
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. |
20dd55f to
76f740e
Compare
|
friendly ping @bnoordhuis (。・∀・)ノ |
|
Ping @bnoordhuis PTAL |
|
Ping @bnoordhuis your comment seems to be addressed. |
|
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
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>
|
Landed in fbb9eef |
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>
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>
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>
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>
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>
some are detected by eslint.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)