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

tools: fix exit code when linting from CI by mscdex · Pull Request #6412 · nodejs/node · GitHub

/ node Public

tools: fix exit code when linting from CI - #6412

Closed
mscdex wants to merge 1 commit into
nodejs:masterfrom
mscdex:tools-fix-jslint-ci-exit-code
ConversationCommits (1)ChecksFiles changed
Closed

tools: fix exit code when linting from CI#6412
mscdex wants to merge 1 commit into
nodejs:masterfrom
mscdex:tools-fix-jslint-ci-exit-code

Conversation

mscdex commented Apr 27, 2016

Copy link
Copy Markdown
Contributor
Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)
  • tools
Description of change

Before this, if there were lint errors reported by make jslint-ci, the process would still exit with an exit code of zero.

This commit fixes that to align with make jslint (exit with code 1 on lint errors).

mscdex added the tools Issues and PRs related to the tools directory. label Apr 27, 2016

mscdex commented Apr 27, 2016

Copy link
Copy Markdown
Contributor Author

/cc @jbergstroem

Trott commented Apr 27, 2016

Copy link
Copy Markdown
Member

This probably should only land after #6411 lands or the CI lint VM is updated to Node.js 6.0.0.

tools: fix exit code when linting from CI
c4d4423 
Before this, if there were lint errors reported by `make jslint-ci`,
the process would still exit with an exit code of zero.

This commit fixes that to align with `make jslint` (exit with
non-zero on lint errors).
mscdex force-pushed the tools-fix-jslint-ci-exit-code branch from 7052427 to c4d4423 Compare April 27, 2016 06:01

mscdex commented Apr 27, 2016

Copy link
Copy Markdown
Contributor Author

I've pushed some additional changes to exit early when a worker dies unexpectedly (e.g. in case of a SyntaxError or similar when eslint actually starts linting files inside a worker).

phillipj commented Apr 27, 2016

Copy link
Copy Markdown
Member

LGTM

2 similar comments

jbergstroem commented Apr 27, 2016

Copy link
Copy Markdown
Member

LGTM

santigimeno commented Apr 28, 2016

Copy link
Copy Markdown
Member

LGTM

jasnell commented Apr 28, 2016

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/2413/
LGTM if CI is green

Trott commented Apr 28, 2016

Copy link
Copy Markdown
Member

@jasnell CI for this won't pass until #6411 lands (or changes are made to the CI linting VM).

jasnell commented Apr 28, 2016

Copy link
Copy Markdown
Member

thanks for the heads up. may want to indicate that in the PR description :-)

Trott commented Apr 28, 2016

Copy link
Copy Markdown
Member

@jasnell Additional complication is that requirement is only true on master. v4.x can get this PR right now and it will be just fine.

Trott commented Apr 29, 2016

Copy link
Copy Markdown
Member

#6411 landed so this can be run in CI now.

Ci: https://ci.nodejs.org/job/node-test-pull-request/2427/

Trott pushed a commit to Trott/io.js that referenced this pull request Apr 29, 2016
tools: fix exit code when linting from CI
1264cec 
Before this, if there were lint errors reported by `make jslint-ci`,
the process would still exit with an exit code of zero.

This commit fixes that to align with `make jslint` (exit with
non-zero on lint errors).

PR-URL: nodejs#6412
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Phillip Johnsen <johphi@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

Trott commented Apr 29, 2016

Copy link
Copy Markdown
Member

Landed in 1264cec

Trott closed this Apr 29, 2016
mscdex deleted the tools-fix-jslint-ci-exit-code branch April 29, 2016 17:44
Fishrock123 pushed a commit that referenced this pull request May 4, 2016
tools: fix exit code when linting from CI
9f23cb2 
Before this, if there were lint errors reported by `make jslint-ci`,
the process would still exit with an exit code of zero.

This commit fixes that to align with `make jslint` (exit with
non-zero on lint errors).

PR-URL: #6412
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Phillip Johnsen <johphi@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
This was referenced May 4, 2016
Closed
Merged
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request May 4, 2016
tools: fix exit code when linting from CI
ddfd737 
Before this, if there were lint errors reported by `make jslint-ci`,
the process would still exit with an exit code of zero.

This commit fixes that to align with `make jslint` (exit with
non-zero on lint errors).

PR-URL: nodejs#6412
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Phillip Johnsen <johphi@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins commented Jun 1, 2016

Copy link
Copy Markdown
Contributor

@mscdex safe to assume this would be included with other linter changes if we were to backport?

mscdex commented Jun 1, 2016

Copy link
Copy Markdown
Contributor Author

@thealphanerd if by 'linter changes' you mean the addition of tools/jslint.js and PRs related to that, then yes.

MylesBorins commented Jun 2, 2016

Copy link
Copy Markdown
Contributor

that is what I mean

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