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

build: add working lint-ci make target (v0.12) #9151

Closed
wants to merge 2 commits into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Oct 18, 2016

Make linting work again on v0.12! We've been missing this since we switched to lint-ci on Jenkins but nobody noticed (cause I'm pretty much the only one who runs it and I'd forgotten that it was only v0.10 that should fail linting).

/cc @nodejs/build

rvagg added 2 commits October 18, 2016 18:56
PR-URL: nodejs#9107
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v0.12 labels Oct 18, 2016
@rvagg
Copy link
Member Author

rvagg commented Oct 18, 2016

CI @ https://ci.nodejs.org/job/node-test-pull-request/4553/ with a happy linter!

fwiw I also cherry-picked over #9107 to v0.12-staging to fix a linting problem that's common with v0.10.

@jasnell
Copy link
Member

jasnell commented Oct 18, 2016

LGTM

Copy link
Member

@jbergstroem jbergstroem left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -585,6 +585,17 @@ cpplint:

lint: jslint cpplint

CONFLICT_RE=^>>>>>>> [0-9A-Fa-f]+|^<<<<<<< [A-Za-z]+
lint-ci: jslint cpplint
@if ! ( grep -IEqrs "$(CONFLICT_RE)" benchmark deps doc lib src test tools ) \
Copy link
Member

Choose a reason for hiding this comment

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

The SmartOS man-page didn't show all flags passed to grep being available, but it seems to work fine anyway.

jasnell pushed a commit that referenced this pull request Oct 28, 2016
PR-URL: #9151
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Oct 28, 2016

Landed in a47fd45

@jasnell jasnell closed this Oct 28, 2016
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Dec 22, 2016
PR-URL: nodejs/node#9151
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants