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

Separate exit code for error #9384

Closed
Soullivaneuh opened this Issue Oct 3, 2017 · 21 comments

Comments

Projects
7 participants
@Soullivaneuh

Soullivaneuh commented Oct 3, 2017

Tell us about your environment

  • ESLint Version: Any
  • Node Version: 6
  • npm Version: None, I use Yarn latest.

What parser (default, Babel-ESLint, etc.) are you using? default

Please show your full configuration:

Not relevant.

What did you do? Please include the actual source code causing the issue.

Running an eslint instance which should crash (config error, intern failing like a permission issue...)

What did you expect to happen?

Having a different exist code issue.

What actually happened? Please include the actual, raw output from ESLint.

The exit code issue is the same as a well running eslint with rule violation detection.

Currently, eslint give the same exit code when some lint rules are not respected and when the binary is failing (example: When the provided configuration file does not exists).

On a automated script, we can't see the difference.

It would be great to have different exit codes:

  • 1 For not respected lint rules.
  • 2 When eslint is failing

Thanks.

@eslintbot eslintbot added the triage label Oct 3, 2017

@platinumazure

This comment has been minimized.

Member

platinumazure commented Oct 3, 2017

I'm 👍 to this, but the team has been resistant to doing this on the grounds that it could be an unnecessarily painful breaking change.

I'm adding to the TSC agenda, but feel free to remove if enough people are 👎 on the issue.

@platinumazure

This comment has been minimized.

Member

platinumazure commented Oct 3, 2017

TSC Summary: It is not desirable that an automated script cannot easily tell the difference between a lint failure and when ESLint itself fails for other reasons. Suggestion is to use different nonzero exit codes for those scenarios. However, in the past this has been identified as a painful change for users.

TSC Question: Could we consider adding this in a major release (i.e., for 5.0)?

@Soullivaneuh

This comment has been minimized.

Soullivaneuh commented Oct 3, 2017

Thanks for your quick answer @platinumazure

I don't really see how this change is painful for the users, but if it's the case, this could indeed be done on the next major.

IMHO, this is a must have and the different exit code system was already implemented on other tools like your. See an example here: https://github.com/FriendsOfPHP/PHP-CS-Fixer#exit-codes

Noob question: What is TSC? 😛

@Soullivaneuh

This comment has been minimized.

Soullivaneuh commented Oct 3, 2017

Another example with yamllint:

$ yamllint app/config/config.yml 
app/config/config.yml
  1:1       warning  missing document start "---"  (document-start)
  2:8       error    too many spaces inside braces  (braces)
  2:33      error    too many spaces inside braces  (braces)
  3:8       error    too many spaces inside braces  (braces)
  3:31      error    too many spaces inside braces  (braces)
  4:8       error    too many spaces inside braces  (braces)
  4:29      error    too many spaces inside braces  (braces)
  5:8       error    too many spaces inside braces  (braces)
  5:37      error    too many spaces inside braces  (braces)
  6:8       error    too many spaces inside braces  (braces)
  6:42      error    too many spaces inside braces  (braces)
  7:8       error    too many spaces inside braces  (braces)
  7:31      error    too many spaces inside braces  (braces)
  8:8       error    too many spaces inside braces  (braces)
  8:28      error    too many spaces inside braces  (braces)
  8:31      warning  too few spaces before comment  (comments)
  9:8       error    too many spaces inside braces  (braces)
  9:28      error    too many spaces inside braces  (braces)
  9:31      warning  too few spaces before comment  (comments)
  10:8      error    too many spaces inside braces  (braces)
  10:26     error    too many spaces inside braces  (braces)
  19:6      warning  missing starting space in comment  (comments)
$ echo $?
1
$ yamllint 
usage: yamllint [-h] [-c CONFIG_FILE | -d CONFIG_DATA]
                [-f {parsable,standard}] [-s] [-v]
                FILE_OR_DIR [FILE_OR_DIR ...]
yamllint: error: too few arguments
$ echo $?
2
@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Oct 3, 2017

Previous discussions: #1795, #2949

If we're distinguishing error codes we would also probably want to have a separate code for "ESLint unexpectedly crashed".

@Soullivaneuh

This comment has been minimized.

Soullivaneuh commented Oct 3, 2017

If we're distinguishing error codes we would also probably want to have a separate code for "ESLint unexpectedly crashed".

This is what I meant by "When eslint is failing", isn't it?

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Oct 3, 2017

I think there are three types of error scenarios here:

  1. ESLint runs successfully, and reports linting problems.
  2. ESLint fails to run due to user error (e.g. a missing configuration file)
  3. ESLint fails to run due to an unexpected internal condition (e.g. a bug in a rule that causes an error to be thrown).

Since you gave a missing configuration file as an example, I'm assuming you are using "When eslint is failing" to refer to category 2. I'm saying that it might be worth distinguishing between expected failures caused by user error, and unexpected failures caused by potential bugs.

@ilyavolodin

This comment has been minimized.

Member

ilyavolodin commented Oct 4, 2017

One more category: ESLint failed to parse your file at all. But in general I'm not sure I like this idea. CIs are usually setup once. Figuring out that config is missing should be part of that initial setup. If config file was there at the beginning, but missing after a while, then it's an error that should break the build and somebody needs to look into it (or maybe the build isn't setup properly). Can you explain the scenario when this change might be useful? Also, could you update initial post to use our change request template?

P.S. TSC stands for Technical Steering Committee, and any core change has to be approved by it.

@Soullivaneuh

This comment has been minimized.

Soullivaneuh commented Oct 6, 2017

Can you explain the scenario when this change might be useful?

Simple: I'm writing a script that's will tell if the code on a specific branch is good or not after a push, on a web interface.

Currently, if eslint is failing, I can't see the difference and my script will just tell the code is wrong. It will be a false positive issue.

At this time, this is the only linter I found not returning a different exit code.

Also, could you update initial post to use our change request template?

Done.

@platinumazure

This comment has been minimized.

Member

platinumazure commented Oct 6, 2017

I'm 👍 for having as many exit codes as we need for different scenarios. And I think it would be good to clarify when changing or adding an exit code should be semver-minor vs semver-major and allow ourselves to make changes when we really need to.

@ilyavolodin You note that CI is usually set up once and then forgotten. By the same token, if we made a change to exit codes (hopefully with semver-major release), CI would only need to be tweaked once, and then forgotten once again. I think this team overestimates the pain of dealing with exit code changes (as long as we don't do so frequently and we do so under the correct type of semver release).

@ilyavolodin

This comment has been minimized.

Member

ilyavolodin commented Oct 7, 2017

@platinumazure I wasn't referring to the pain of switching exit codes, I'm just not sure I understand the reasoning behind it. A build failure is a build failure and somebody should look at it. To me, it really doesn't matter if the build failed due to code issue, or due to config missing. But maybe it's just me, not sure.

@platinumazure

This comment has been minimized.

Member

platinumazure commented Oct 7, 2017

@ilyavolodin

This comment has been minimized.

Member

ilyavolodin commented Oct 8, 2017

Very well, I'm still not sure that making a breaking change for something like this is justified, but I'm not going to oppose it (as part of major release, and 1 should stay a general error reported by ESLint rules for consistency).

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Oct 13, 2017

The TSC did not reach consensus on this issue in today's meeting, so I'm leaving it on the agenda for next meeting.

@ilyavolodin

This comment has been minimized.

Member

ilyavolodin commented Oct 16, 2017

We have a few people (including myself) who don't feel like this is important enough to include in the core, but don't feel strongly against it, and will not block the vote. As such, we would like input from other member of the TSC. @nzakas @alberto and @gyandeeps what are your feeling about this feature?

@Soullivaneuh

This comment has been minimized.

Soullivaneuh commented Oct 16, 2017

Well, suppose there's some custom handling for actually reporting the CI
error. Maybe that would depend on an exit code for whether to message the
developer about their code being bad, or the IT group about an environment
issue.

Exactly and for my personal use case: I'm writing a SaaS service which run some lint tools like yours and will get the lint report output.

Currently, because I can't make the difference between exit codes, I can't see if the lint was failing or not and will just get an output parsing error if the tool failed.

@alberto

This comment has been minimized.

Member

alberto commented Oct 18, 2017

I say 👍

@nzakas

This comment has been minimized.

Member

nzakas commented Oct 25, 2017

Traditionally, we have decided against multiple exit codes because there hasn't been a lot of evidence that multiple exit codes provide enough value to risk breaking existing integrations that might have logic looking only for exit code 1 (like if (exitCode != 1)). My opinion has generally been the same as Ilya's in that if something the lint fails for any reason, then you need to look at it, so it only matters that the exit code isn't zero.

As a reminder, we had an ill-fated experiment where we (actually me) changed the exit code to be the number of lint errors. We had to roll that back because it broke a lot of things, especially when the number of problems was > 254. That's part of why we've been so reluctant to change exit codes since then.

That said, I can see an argument for making this change as the output for limiting problems vs. a crash is different, and I can see it being helpful for tools to be able to know how to bubble up the error details.

So I'm 👍 for keeping exit code 1 for linting errors and introducing exit code 2 for any other type of errors. That would be a breaking change and would have to go in on a major release. I wouldn't want separate exit codes for every type of error condition as I think that's overkill.

@not-an-aardvark not-an-aardvark added this to Accepted in v5.0.0 Nov 3, 2017

@Soullivaneuh

This comment has been minimized.

Soullivaneuh commented Nov 22, 2017

@nzakas It's a great news! 👍

That would be a breaking change and would have to go in on a major release.

Maybe we can propose this change as an option for current stable to keep BC and then remove the option on next major?

@platinumazure

This comment has been minimized.

Member

platinumazure commented Dec 29, 2017

Maybe we can propose this change as an option for current stable to keep BC and then remove the option on next major?

I would be okay with that, but I'm not sure if this would require separate TSC discussion to approve adding a CLI option.

@eslint/eslint-tsc Any thoughts on creating an option that would change exit codes for users who want to opt in (releasing that as semver-minor), and then changing the default exit code behavior and removing the option in 5.0? Does that need another TSC motion given that we've already accepted the breaking change in 5.0?

@ilyavolodin

This comment has been minimized.

Member

ilyavolodin commented Dec 29, 2017

I would be against doing that. Introducing an option now just to remove it in a few month doesn't seem like a good idea. I understand @Soullivaneuh desire to have a working solution now, but it feels like it's a lot more work for us to do that (introduce a new option, retire a new option, move the code around, etc.).

not-an-aardvark added a commit that referenced this issue Feb 23, 2018

@not-an-aardvark not-an-aardvark moved this from Accepted, ready to implement to Ready to merge in v5.0.0 Feb 23, 2018

v5.0.0 automation moved this from Ready to merge to Done Mar 22, 2018

@eslint eslint bot locked and limited conversation to collaborators Sep 19, 2018

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