FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tools: remove default parameters from lint rule by Trott · Pull Request #6411 · nodejs/node · GitHub

/ node Public

tools: remove default parameters from lint rule - #6411

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:fixit
ConversationCommits (1)ChecksFiles changed
Closed

tools: remove default parameters from lint rule#6411
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:fixit

Conversation

Trott commented Apr 27, 2016

Copy link
Copy Markdown
Member
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.

tools: remove default parameters from lint rule
dab9658 
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 added the tools Issues and PRs related to the tools directory. label Apr 27, 2016

Trott commented Apr 27, 2016

Copy link
Copy Markdown
Member Author

/cc @mscdex @jbergstroem @thealphanerd

mscdex commented Apr 27, 2016

Copy link
Copy Markdown
Contributor

LGTM

Trott commented Apr 27, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

Trott mentioned this pull request Apr 27, 2016
Closed
2 tasks

cjihrig commented Apr 27, 2016

Copy link
Copy Markdown
Contributor

LGTM

1 similar comment

ChALkeR commented Apr 27, 2016

Copy link
Copy Markdown
Member

LGTM

evanlucas commented Apr 27, 2016

Copy link
Copy Markdown
Contributor

LGTM, probably needs a CI run though :]

jasnell commented Apr 27, 2016

Copy link
Copy Markdown
Member

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

Trott commented Apr 27, 2016

Copy link
Copy Markdown
Member Author

@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 commented Apr 27, 2016

Copy link
Copy Markdown
Member

LGTM

Trott commented Apr 28, 2016

Copy link
Copy Markdown
Member Author

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

Trott commented Apr 29, 2016

Copy link
Copy Markdown
Member Author

Only CI failure is a known unrelated problem.

jasnell commented Apr 29, 2016

Copy link
Copy Markdown
Member

LGTM

Trott added a commit to Trott/io.js that referenced this pull request Apr 29, 2016
tools: remove default parameters from lint rule
c8e137b 
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 commented Apr 29, 2016

Copy link
Copy Markdown
Member Author

Landed in c8e137b

Trott closed this Apr 29, 2016
Fishrock123 pushed a commit that referenced this pull request May 4, 2016
tools: remove default parameters from lint rule
e62c42b 
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>
This was referenced May 4, 2016
Closed
Merged
Trott deleted the fixit branch January 13, 2022 22:43
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

tools Issues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL