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

Option to exit with success even if there are errors #2949

Closed
simonewebdesign opened this Issue Jul 8, 2015 · 37 comments

Comments

Projects
None yet
10 participants
@simonewebdesign

simonewebdesign commented Jul 8, 2015

Hi, we are using the eslint CLI together with npm for running eslint from the command line.

So far so good, apart that we're also using parallelshell to run commands in parallel. The problem is, parallelshell kills all the running processes if one of them returns an exit status that's not 0.

We managed to work around this by exiting manually:

eslint source ; exit 0

But since it's quite ugly, is it possible to add an option to exit with 0 even on errors (like if they were just warnings)?

Thanks!

@gyandeeps gyandeeps added the triage label Jul 8, 2015

@michaelficarra

This comment has been minimized.

Member

michaelficarra commented Jul 8, 2015

👎 just use what you are currently using. It's no more ugly than this unnecessary option.

@simonewebdesign

This comment has been minimized.

simonewebdesign commented Jul 8, 2015

The fact is, what if there's an actual error on eslint? With exit 0 we won't even notice it I guess...

I was thinking something like eslint --no-exit-on-error would be nice to have, however it's just an idea.

@nzakas

This comment has been minimized.

Member

nzakas commented Jul 8, 2015

This is the reason we allow you to configure warnings, to indicate you dont want the exit code to change. Have you considered just setting all rules to be warnings instead of errors?

@simonewebdesign

This comment has been minimized.

simonewebdesign commented Jul 8, 2015

Yeah we've considered that, but isn't it worth having an option from the command line? I can give it a try and submit a PR, what do you think?

@nzakas

This comment has been minimized.

Member

nzakas commented Jul 8, 2015

I dont believe such an option makes sense. As I mentioned, we allow you to control the exit code via rule config. I'm not sure why anything more than that is necessary.

Is there a reason you aren't setting rules to be warnings?

@simonewebdesign

This comment has been minimized.

simonewebdesign commented Jul 9, 2015

No, there's no reason really. It's just that I found it more straightforward to just exit 0 - do you think I'm going to run into any issues if I keep doing that?

@nzakas

This comment has been minimized.

Member

nzakas commented Jul 10, 2015

If you want to do it that way, it's up to you. In any event, I don't see us implementing such an option, so I'd suggest keep using an external script or switch your rules to be all warnings.

@nzakas nzakas closed this Jul 10, 2015

@BYK

This comment has been minimized.

Member

BYK commented Jul 15, 2015

@nzakas - this is also a problem for us. We are using Phabricator which thinks the process had trouble running when it exits with an error code. Now on why we cannot make everything a warning is because we actually use the distinction between errors and "suggestions" where an "error" is a blocker (before submitting a patch) a "warning" is more of an advice but not enforced.

Our current workaround is using a "proxy" file which does require('eslint/lib/cli').execute(process.argv); to both exit with an error code if something really breaks and silence the "lint error" case. That said I see lib/cli is also going away making this a bit harder.

Does that make sense?

@nzakas

This comment has been minimized.

Member

nzakas commented Jul 15, 2015

Does that mean ESLint is running in two systems? Phabricator and something else?

How do you flag errors if the exit code is always zero?

@BYK

This comment has been minimized.

Member

BYK commented Jul 15, 2015

Does that mean ESLint is running in two systems? Phabricator and something else?

We also have an npm command to run lint on everything manually but it mainly runs with Phabricator yeah. That said even if it was running on CI, most CI systems treat non-zero error code as a complete failure. We had the same problem with our unit tests for instance.

How do you flag errors if the exit code is always zero

We parse the output. Just as we do for test results.

@nzakas

This comment has been minimized.

Member

nzakas commented Jul 15, 2015

Hmm OK, let's think about this one some more. Not sure if the blunt instrument of a forced exit code is the best approach. It seems like what we want is a way of saying to treat all errors as warnings. Need to think about this a bit, open to suggestions.

@nzakas nzakas reopened this Jul 15, 2015

@ilyavolodin

This comment has been minimized.

Member

ilyavolodin commented Jul 16, 2015

How about a compromise? If --output-file attribute is passed on CLI, exit code should be 0 no matter the result of the linting.

@BYK

This comment has been minimized.

Member

BYK commented Jul 16, 2015

It seems like what we want is a way of saying to treat all errors as warnings. Need to think about this a bit, open to suggestions.

I mean, as I tried to mention before, we need the separation of the notion of error and warning in the output. May be a flag like --ignore-errors or something to only suppress the non-zero output code for lint errors (but nothing else) may work?

How about a compromise? If --output-file attribute is passed on CLI, exit code should be 0 no matter the result of the linting.

This doesn't sound like a bad idea. I think I can make Phabricator to work with a file. That said stdout is always preferred.

@IanVS

This comment has been minimized.

Member

IanVS commented Jul 16, 2015

It seems like what we want is a way of saying to treat all errors as warnings.

This might also be something nice to have for projects which are beginning to implement ESLint. They may want to commit their desired .eslintrc with a mixture of errors and warnings, but then start off with all errors overridden to warnings so that fixes can be made over time until they are ready to start breaking builds on errors, at which point it would be a matter of changing one option rather than changing the rule configuration.

@nzakas

This comment has been minimized.

Member

nzakas commented Jul 16, 2015

@ilyavolodin that's a bit too magical for my tastes - it would be hard to explain why people need to create an output file just to get a 0 exit code.

What about a --severity flag that can override the severity of rules? So --severity 1 forces everything to be a warning and --severity 2 forces everything to be an error?

@BYK

This comment has been minimized.

Member

BYK commented Jul 16, 2015

What about a --severity flag that can override the severity of rules? So --severity 1 forces everything to be a warning and --severity 2 forces everything to be an error?

Would that also affect the output or just the exit code?

@nzakas

This comment has been minimized.

Member

nzakas commented Jul 17, 2015

It would affect the output in that everything would say "warning" or "error" based on the forced severity.

@BYK

This comment has been minimized.

Member

BYK commented Jul 17, 2015

Then it definitely wouldn't help our case. Might be useful for others though, I don't know.

@simonewebdesign

This comment has been minimized.

simonewebdesign commented Jul 17, 2015

It would be helpful for us. We don't mind the distinction between errors and warnings, so we'd use --severity 1 to just treat everything as a warning.

@nzakas

This comment has been minimized.

Member

nzakas commented Jul 17, 2015

@BYK can you explain why it wouldn't work for you?

@BYK

This comment has been minimized.

Member

BYK commented Jul 17, 2015

@nzakas - Because we do use the difference between an error and a warning. Warnings are shown as "advice" to the committer where as errors are blockers. The only reason we need a 0 exit code from the linter is because the software we are using assumes the process failed if it exits with a non-zero code.

It always parses the results to see if there are errors or not so the exit code is reserved only for lower-level failures like a parse error or process shutting down due to I/O error etc.

@nzakas

This comment has been minimized.

Member

nzakas commented Jul 18, 2015

Hmm OK. I think the larger question here is if we are saying "ignore errors" or "set exit code". I'd there a scenario where we would want to arbitrarily set the exit code to something other than 0?

@gyandeeps

This comment has been minimized.

Member

gyandeeps commented Jul 18, 2015

Long long time back, i had the need and then I created gruntify-eslint which had an silent option to not exit with code 1. I know at that time the CLIEngine was not available.

@BYK

This comment has been minimized.

Member

BYK commented Jul 27, 2015

Hmm OK. I think the larger question here is if we are saying "ignore errors" or "set exit code". I'd there a scenario where we would want to arbitrarily set the exit code to something other than 0?

I think setting the exit code directly or forcing 0 is a bit useless. As far as I can understand a non-zero exit code indicates something went wrong when running the program and linting failed completely instead of indicating there were errors. At least that's what Arcanist thinks.

I guess a potential workaround for this can simply be writing a linter wrapper in Arcanist (which is quite common) and simply check for the exit code: if it's 1, don't worry and just parse the results, if not freak out.

@nzakas

This comment has been minimized.

Member

nzakas commented Jul 27, 2015

@BYK Are you saying you don't think this option is necessary?

@BYK

This comment has been minimized.

Member

BYK commented Jul 27, 2015

@nzakas - I'm saying if we cannot converge on a common ground for such an option (or a complete change of behavior) there is an alternative way for a specific use case, that is Arcanist. I don't think forcing a 0 exit code even if something else fails (such as file read errors or parsing errors) is valuable.

@nzakas

This comment has been minimized.

Member

nzakas commented Jul 27, 2015

Okay, at this point we're going in circles and I don't see a resolution forthcoming. So, I'm closing and recommending that people create a wrapper around ESLint to do with the exit code as you please.

@rhbecker

This comment has been minimized.

rhbecker commented Jan 7, 2016

tl;dr

Might it make sense to use at least 2 distinct non-zero exit codes in order to reflect at least 2 distinct "error" scenarios?

(at least) 2 distinct scenarios

  1. eslint executed successfully, but found rule violations in the linted set of files
  2. eslint did not execute successfully

in the code

Just casually looking at cli.js, I'd think the exit code returned on line 137 - in the catch block of option parsing - is a case of scenario 2, and should be distinct from the exit code returned on line 180, when (report.errorCount || tooManyWarnings) evaluates to true, which is a case of scenario 1. Lines 157 and 182 also appear to be scenario 2 cases.

analogy

Returning 1 for every type of "error" is like returning 404 for every http "error". Clients need to know whether the "error" is client-fixable -4xx class - or requires a server fix - 5xx class, and if the former, the nature of the fix required (authenticate, try a different URI, etc.).

my immediate use case

I'm wanting to use the eslint binary in a npm run-script. As things stand, a rule violation (scenario 1) causes npm to dump debug output to the console, and to a log. I realize I can "scripts": { "lint": "eslint . || exit 0" }, but swallowing all non-zero exit codes means I won't, for example, know that I'm using some old flag that you've deprecated, or that my eslint installation is somehow corrupted.

(If anyone reading this happens to know how to do something like eslint -x; if [ $? -gt 0 ]; then echo "hello"; fi in an npm run-script, please ping me!)

"solutions"

Any distinction in the exit codes is good enough for me, and others are probably better qualified to suggest a "best practice", but just to spark conversation ...

  1. make rule violations exit with 1 and all other "errors" exit with 2 (clients can then choose to test whether exit code is > 1
  2. add an option flag where a client can set the exit code for one or both scenarios
@rhbecker

This comment has been minimized.

rhbecker commented Jan 7, 2016

see also #858, #2409

@nzakas

This comment has been minimized.

Member

nzakas commented Jan 8, 2016

I'm sorry, we decided not to pursue this any further. As I mentioned in my last comment, we recommend wrapping ESLint in something else if you want more control over exit codes.

@rhbecker

This comment has been minimized.

rhbecker commented Jan 8, 2016

That's exactly what I'm trying to do, but as far as I can see, a wrapper isn't going to help me if you're always returning 1 for every class of error.

@ilyavolodin

This comment has been minimized.

Member

ilyavolodin commented Jan 8, 2016

I think what @nzakas means is that wrapper should be using our Node.js API, not CLI, this way you will get all of the information you need.

@rhbecker

This comment has been minimized.

rhbecker commented Jan 8, 2016

@ilyavolodin Didn't know that was an option. Thanks for the tip.

If anyone has any example code that leverages the Node.js API via npm run-script, don't be shy in sharing!

adamkasztenny added a commit to adamkasztenny/eslint that referenced this issue Aug 13, 2016

New: Add CLI option for exiting 0 (fixes eslint#2949)
Normally, if checks produce errors, ESLint exits with a status code of 1.
With this option, it will always exit 0, even if there are errors.
However, if ESLint fails internally (e.g. config is not in the proper format)
it will still exit with a nonzero status code

adamkasztenny added a commit to adamkasztenny/eslint that referenced this issue Aug 13, 2016

New: Add CLI option for exiting 0 (fixes eslint#2949)
Normally, if checks produce errors, ESLint exits with a status code of 1.
With this option, it will always exit 0, even if there are errors.
However, if ESLint fails internally (e.g. config is not in the proper format)
it will still exit with a nonzero status code
@platinumazure

This comment has been minimized.

Member

platinumazure commented Aug 13, 2016

Can this be reopened, strictly in terms of exiting with a different (non-zero) status code for "successful lint with errors" vs "ESLint threw an exception"? It doesn't result in any magical success exit code when there were lint errors but it does help consumers who have to rely on the CLI distinguish between those cases.

@nzakas

This comment has been minimized.

Member

nzakas commented Aug 15, 2016

@platinumazure that's only tangentially related to this issue, and we can't change that behavior anyway. We actually tried changing exit codes at one point and it caused massive problems for CI systems, so I don't think it's worthwhile to pursue.

@platinumazure

This comment has been minimized.

Member

platinumazure commented Aug 15, 2016

@nzakas If it's a "massive problem for CI systems", can we do this as a breaking change with 4.0.0?

@timfish

This comment has been minimized.

timfish commented Jan 14, 2017

tslint has:

--force     return status code 0 even if there are lint errors

I'm pretty sure tslint also lints JavaScript now...

@lorensr lorensr referenced this issue Jul 12, 2017

Closed

Modify script lint #652

3 of 3 tasks complete

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.