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: remove default parameters from lint rule #6411

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Apr 27, 2016

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

tools

Description of change

The CI server uses system Node.js for linting, which is currently v5.x.
So default parameters are not supported there. This change removes the
default parameters.

The CI server uses system Node.js for linting, which is currently v5.x.
So default parameters are not supported there. This change removes the
default parameters.
@Trott Trott added the tools Issues and PRs related to the tools directory. label Apr 27, 2016
@Trott
Copy link
Member Author

Trott commented Apr 27, 2016

/cc @mscdex @jbergstroem @thealphanerd

@mscdex
Copy link
Contributor

mscdex commented Apr 27, 2016

LGTM

@Trott
Copy link
Member Author

Trott commented Apr 27, 2016

Arguments for expediting this:

  • Falls under the "trivial change" exception
  • CI is kinda-sorta broken (still green, but shouldn't be!) while this isn't merged

Arguments against expediting this:

  • Nothing is really broken so what's the rush?
  • By the time we finish debating expedite vs. not expediting, the 48 hours will be up

@cjihrig
Copy link
Contributor

cjihrig commented Apr 27, 2016

LGTM

1 similar comment
@ChALkeR
Copy link
Member

ChALkeR commented Apr 27, 2016

LGTM

@evanlucas
Copy link
Contributor

LGTM, probably needs a CI run though :]

@jasnell
Copy link
Member

jasnell commented Apr 27, 2016

@Trott is this a problem for v5 and v4 also?

@Trott
Copy link
Member Author

Trott commented Apr 27, 2016

@jasnell It was already removed in the backport for LTS so it shouldn't be a problem for v4. It's unclear to me when/how this would land in v5, so I'm less certain there.

@jbergstroem
Copy link
Member

LGTM

@Trott
Copy link
Member Author

Trott commented Apr 28, 2016

@Trott
Copy link
Member Author

Trott commented Apr 29, 2016

Only CI failure is a known unrelated problem.

@jasnell
Copy link
Member

jasnell commented Apr 29, 2016

LGTM

Trott added a commit to Trott/io.js that referenced this pull request Apr 29, 2016
The CI server uses system Node.js for linting, which is currently v5.x.
So default parameters are not supported there. This change removes the
default parameters.

PR-URL: nodejs#6411
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented Apr 29, 2016

Landed in c8e137b

@Trott Trott closed this Apr 29, 2016
Fishrock123 pushed a commit that referenced this pull request May 4, 2016
The CI server uses system Node.js for linting, which is currently v5.x.
So default parameters are not supported there. This change removes the
default parameters.

PR-URL: #6411
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott Trott deleted the fixit branch January 13, 2022 22:43
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.

7 participants