Make CLI not to return non-zero exist code when only warnings occur

[aleung]

User story.

As a continuous integration user, I can provide a parameter to CLI, so that the CI pipeline won't fail when there is no error but only warnings.

As a continuous integration user, I can provide a parameter to the CLI, so that only results of a certain severity will be returned as output.

Describe the solution you'd like

Updated 2019-07-16 by @philsturgeon

Two new CLI options:

1. --fail-severity=warn - enum[hint, info, warn, error] - change the level of results which come out of Spectral.
2. --display-only-fail-severity-results - boolean - spectral will error if results of any level at all are reported. We should make that configurable.

We should probably not change the default behavior in this as its aiming for v4.1. Place a TODO note in the code to change the default to warn in v5.0.

[philsturgeon]

I’m open to something along these lines, but not exactly what ESLint does. I think number of warnings is arbitrary, if you’re asking for feedback on your specs and you get some then you should fix it, and if you don’t like what feedback you’re getting use skip, make a custom ruleset, or extend the default ruleset to change the severity of a specific rule.

With that said, can you think of a switch? —error-only or something?

[aleung]

@philsturgeon --error-only is fine for me. I just thought --max-warnings would give more flexibility to some other users.

[soulcutter]

--ignore-warnings maybe?

I mildly favor a boolean-style flag vs tracking how many warnings trigger an error. It's lower-complexity, and also I am skeptical of the use case where 3 warnings is ok, but 4 is an error.

[philsturgeon]

I prefer a Boolean flag too but —ignore-warnings could be a tad confusing. We’re not ignoring them, just not failing the run if we see them. Ignoring might be closer to #381.

Should these two features perhaps be the same feature?

We could have —severity=error and it will only show errors, and only fail if errors come back.

—severity=warm would show warnings and errors, and fail if either came back.

—severity=info would show info, warnings, errors, and fail if... info came back? Or warnings/errors?

Not sure on the last one.

[philsturgeon]

There could be two switches, —fail-severity=warn —severity=info

First is where it fails, second is what it shows. Both have same defaults they have now.

[XVincentX]

I had a look to some other tools on the market that deal with the same situation; mscpp (Microsoft's C/C++ compiler) has a dedicated flag (/WX, if I recall correctly) to treat the warnings as an error, and you can even specify which warning should be treated as errors.

[philsturgeon]

We can currently specify which warnings should be treated as errors with ruleset extension and changing the severity level in the ruleset file. This might be a slightly different thing?

[XVincentX]

Yeah, but still there's an option on the compiler itself to treat all the warnings as errors.

I've also checked out GCC, and they do basically the same thing.

[philsturgeon]

Right, you were saying two things and I replied to one of the things :D

... you can even specify which warning should be treated as errors.

We got that bit covered already.

there's an option on the compiler itself to treat all the warnings as errors.

Converting all warnings into errors is essentially what we are currently doing, as we return a failure status code if we spot a warning. The requester is asking for an option to do the opposite. Does mscapp or GCC have that?

Also im not sure compilers are the closest comparison, maybe other linting tools would be more similar.

I like Rubocop for example. They have a similar two switch approach that I was hinting towards pre-coffee:

• --display-only-fail-level-offenses - Only output offense messages at the specified --fail-level or above
• --fail-level - Minimum severity for exit with error code. Full severity name or upper case initial can be given.

[soulcutter]

--fail-level is my favorite so far. It might be nice to have some consistency between the fail flag and the output flag, sooo... --output-level maybe?

[philsturgeon]

Ok so we have two possible approaches so far:

1. --fail-severity=warn and --output-severity=info
2. --fail-severity=warn and --display-only-fail-level-results (a'la rubocop)

[aleung]

Though you all prefer boolean flag, let me share my use case of ESLint --max-warnings. No going to convince somebody but explain why ESLint provides this kind of option.

Linter wasn't used in my team from day 0. When I introduced it we already had tens of projects and their quality was no so good. You can imagine that the linter reported a large number of errors and warnings. I forced the team to fix errors immediately but the project schedule won't allow us to spend too much time to fix all warnings -- I had carefully define the ruleset to limit the number of error rules. But if there is no any restriction on the number of warnings, the team won't take care of them. So I set max warnings in the CI pipeline to make sure at least the number of warnings won't go up. And also I decreased the number gradually, so that developer had to fix some warnings in each commit but not too many to impact his/her progress. After 2~3 months, the code quality finally reaches a satisfactory status.

Even though, I never set the max warnings to zero but give it a reasonable value -- of cause you can say it's arbitrary. We adjust the value by retrospective of overall product quality. We do not force to fix all warning because in some cases there is reason to break the rule otherwise there will be other negative impacts. (BTW, in ESLint/TSLint there is option to temporarily disable a rule for a line, but it's impossible to do it in Spectral). Working in software enterprise, we have to compromise and get balance between quality, workload, and schedule.

[philsturgeon]

Alright, chatting with @P0lip I think we're aligned that option 2 is the way to go. I'm going to update the initial message with this.

[philsturgeon]

@aleung sorry I didn't see your reply there before I wrote my last one. I had this page open a while I guess.

That's absolutely a valid use case, and I get it, but I think it can be handled in other ways. For now people have --skip-rule and they can extend rulesets to override things, or make them not be errors, there's all sorts of options here, but if this comes up again in the future after we've provided this basic approach, we can revisit.