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

tools: delete an unused argument #14251

Closed
wants to merge 1 commit into from

Conversation

phisixersai
Copy link
Contributor

Delete the argument never being used.

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows [commit guidelines]

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Jul 15, 2017
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green.

@Trott
Copy link
Member

Trott commented Jul 15, 2017

Thanks for the contribution! 🎉

CI: https://ci.nodejs.org/job/node-test-pull-request/9133/

@vsemozhetbyt
Copy link
Contributor

@Trott What about changing args: none into args: after-used?

@Trott
Copy link
Member

Trott commented Jul 15, 2017

@Trott What about changing args: none into args: after-used?

You mean just in the tools directory alone? Sure, although that can be a separate PR. (Might as well do docs at the same time.)

Pretty sure we'll still have tons of errors in benchmark, test, and lib. Small PRs to fix those one or two files at a time would likely be uncontroversial. I have no idea if people would object to the churn of doing them all at once. There used to be more churn objections than there have been lately, so I don't know.

@vsemozhetbyt
Copy link
Contributor

@Trott I shall do some churn proposals soon, including this one.

@Trott
Copy link
Member

Trott commented Jul 15, 2017

@Trott I shall do some churn proposals soon, including this one.

Guess I'll have to find some other source of super-easy first contributions for Code + Learn attendees! 😆

@vsemozhetbyt
Copy link
Contributor

They will be issue proposals, not PR, so you will be able to use them if there will be some consensus)

@Trott
Copy link
Member

Trott commented Jul 15, 2017

They will be issue proposals, not PR, so you will be able to use them if there will be some consensus)

I was half-joking, but truth be told, I was planning on giving some of those out at Code + Learn tomorrow if I run out of other issues (which I don't expect to happen, but it's always good to be prepared!).

@vsemozhetbyt
Copy link
Contributor

@Trott #14253

@gibfahn
Copy link
Member

gibfahn commented Jul 16, 2017

I have no idea if people would object to the churn of doing them all at once.

Doing them all at once is the same amount of code churn, but it's a lot less PR churn. Unless we're doing them in code-and-learns (in which case it's fine), I'd prefer it all at once. It's also easier for backporting (if the person is willing to raise a backport PR).

@tniessen
Copy link
Member

Landed in 2e91d8d, thank you for your contribution and welcome aboard! 🎉

@tniessen tniessen closed this Aug 16, 2017
tniessen pushed a commit that referenced this pull request Aug 16, 2017
PR-URL: #14251
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MSLaguana pushed a commit to nodejs/node-chakracore that referenced this pull request Aug 21, 2017
PR-URL: nodejs/node#14251
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
PR-URL: #14251
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
PR-URL: #14251
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 20, 2017
PR-URL: #14251
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.