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

Add CLI option to treat warnings as errors #3312

Closed
Jeehut opened this issue Aug 20, 2020 · 4 comments
Closed

Add CLI option to treat warnings as errors #3312

Jeehut opened this issue Aug 20, 2020 · 4 comments
Labels
enhancement Ideas for improvements of existing features and rules.

Comments

@Jeehut
Copy link
Collaborator

Jeehut commented Aug 20, 2020

This feature was requested several times, on a quick search I found these related links:
#160, #268, #834, #1249, this SO thread

From what I've read in the discussions there, I observed these things:

  1. There was no reason mentioned not to add support for such an option, most questions from contributors were just trying to understand the underlying problem to provide a good long-term solution.
  2. The --strict option (which makes the whole swiftlint command fail if any warnings appears) clearly seems to be misleading to some who think that it will convert warnings to errors. Others were exactly looking for what --strict does.

I just ran into this problem in a different context: While setting up danger-ruby-swiftlint for a project I noticed that they provide a additional_swiftlint_args option which will pass options like --strict to SwiftLint, but because they are not working directly with the status code but instead use the reported warnings and errors as the source of truth for deciding if they should fail or not, there's currently no way to make the CI fail on warnings if using SwiftLint via that Danger plugin.

While I have reported the issue here and it could be solved without any changes on the side of SwiftLint, I ask myself the question: Why should SwiftLint not just provide what people were asking for several times already? @jpsim even already mentioned in this comment that the solution requires only a single line to be changed (+ the option parsing, of course). Another option can't really hurt, right? And if some people prefer it over --strict, then we should support it. Especially since --strict doesn't work in some use cases as explained in the example above. Don't get me wrong, I'm not suggesting to replace --strict here, but just complement it by this slightly different approach.

I will actually look into this myself and provide a PR, but before doing that I thought I should probably bring that topic up again and maybe discuss what your thoughts are and also what such an option could be named. I think the clearest name is --treat-warnings-as-errors, or a little shorter --warnings-as-errors. But even shorter names like --always-error could also be used. What do you think? Any opinions or suggestions?

@Jeehut Jeehut added the enhancement Ideas for improvements of existing features and rules. label Aug 20, 2020
@Jeehut
Copy link
Collaborator Author

Jeehut commented Oct 2, 2020

FYI: Just implemented this new option with the shorter name suggestion --warnings-as-errors (since all options are quite short) and posted it in #3371. Implementation was pretty straight forward as this option basically does exactly the opposite of the already existing --lenient option, thus I just needed to reverse the conversion of the violation level error to warning. I think it just makes more sense now as we now have --lenient and --warnings-as-errors.

@jpsim
Copy link
Collaborator

jpsim commented Oct 2, 2020

Thanks for doing this. For what it's worth, I think it'd be better to change how --strict works to do what you're suggesting, upgrading all warnings to errors.

It's probably how I should have done it originally.

@Jeehut
Copy link
Collaborator Author

Jeehut commented Oct 3, 2020

@jpsim Sure, no problem, I have just created an alternative PR with #3372 that implements your suggested approach. Note that I personally think that the new one is a breaking change though as it's changing the behavior of an existing command people have already gotten used to. Some workflows which trusted on the exact current behavior might break. But I leave it to you to decide which PR you want to merge, I'm keeping both open. For me, both are fine.

@jpsim
Copy link
Collaborator

jpsim commented Nov 7, 2020

Closed as #3372 has been merged.

@jpsim jpsim closed this as completed Nov 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Ideas for improvements of existing features and rules.
Projects
None yet
Development

No branches or pull requests

2 participants