Adding --strict does not log or display linter violations as errors #268
Comments
|
The only thing that the Warnings are still warnings, but unlike regular SwiftLint operation which exits $ echo "let abc:Int = 0" | swiftlint lint --use-stdin
<nopath>:1:5: warning: Colon Violation: Colons should be next to the identifier when specifying a type. (colon)
Done linting! Found 1 violation, 0 serious in 1 file.
$ echo $?
0
$ echo "let abc:Int = 0" | swiftlint lint --use-stdin --strict
<nopath>:1:5: warning: Colon Violation: Colons should be next to the identifier when specifying a type. (colon)
Done linting! Found 1 violation, 0 serious in 1 file.
$ echo $?
1 |
jpsim
added
the
question
|
In your view, what is the best way to display all violations as errors using SwiftLint? One thing I played around with was modifying the source code to add a 'severity' parameter to the style violation initializer of certain rules (the same way it's being done in the force casting rule implementation) and to pass in .Error to the severity param, which did not work. |
|
If you're building SwiftLint from source, you could hardcode However, I'd love to know what your motivation for this is, so we can come up with a more generally applicable solution within SwiftLint so you don't have to build from source and maintain this patch. |
|
Of course! The primary reason is cultural. People do not take warnings as seriously as they should. Errors, obviously, have to be dealt with immediately. Warnings do not. This isn't because they do not believe in or write high quality code. There are lots of reasons why a project might have a few lingering warnings. To me, the point of linting is introducing a passive tool that communicates a team's values and improves their code. Introducing warnings into a project means that a linter becomes not actually a totally passive tool. Its usage then requires some policing by other team members -- not because people are lazy, but because they have to decide how to allocate their time, and sometimes resolving warnings can't be done in a certain time frame. Also, if an app already has a few warnings (see explanation for reasons why below) they can easily forget whether or not they added warnings until they merge a PR and see if the CI tool suggests that they introduced new warnings into the project. There are lots of reasons why a code base might already have some warnings -- these include needing to integrate 3rd party code which introduces warnings -- sometimes unavoidable on large apps owned by mid sized or large companies -- and the fact that fast moving teams sometimes just need to leave errors in their code temporarily. Moreover, apps that have been in development for many years may have legacy code with warnings that people have not yet found time to fix. That's not the case on the app I work on, but I can absolutely see that happening on other teams. For that reason, having linter violations displayed as warnings does not have the kind of impact you would hope for. In my view, linter violations are far more likely to be dealt with if SourceKit/the compiler provide errors rather than warnings in the IDE. If you implement more fix-its in the SwiftLint project I think this point still stands -- I would like to see errors in my project where linter rules are not respected, and suggestions for changes, like what's built into Xcode using SourceKit. Note: This whole comment is based on a model where the transition to a SwiftLinted project is first only performed on files that have a diff in a developer's current branch, using a build script, and then on the entire project potentially. This helps mitigate the fact that a project that only shows errors, not warnings after being linter, would require potentially tons of changes in order to build. What do you think about this, JP? I'm really interested in your opinion, and in your reasoning for initially not introducing this functionality. |
|
So why don't you get all that from having One thing we could do is have the
We could also introduce a 4th reporter ( Keep in mind that the only difference in all this is the color of the Xcode UI element, which I'm not sure has the cultural impact you're looking for. |
Just to clarify what I was trying to explain before, in a project that has unavoidable warnings that can't, for reasons mentioned above, be resolved immediately, you can't treat all warnings as errors, or you won't be able to compile before they're all resolved. I'm assuming 'treat warnings as errors' prevents compiling with warnings.
This seems like a good idea to me. When speaking with a bunch of people considering linting their Swift projects recently, it seemed that a handful - some Swift developers, some developers working in other languages who have used linters in those languages - were concerned about whether linters will be useful for their teams if they only treat violations as warnings. |
|
Oh, I now understand what you're saying -- strict prevents compilation but does not show errors. |
Rather
If linting is part of their CI, then if they use |
I think that's very useful from a UI standpoint.
That said, I'm really happy this does return an exit code of 1. |
|
You might be right that the entire concept of graded violation severities is unnecessary. That a violation should be binary rather than graded. My initial motivation for introducing it was to distinguish between something that was unequivocally an issue from something more debatably so (either false positives or matters of personal preference). That being said, I don't gather usage metrics on SwiftLint so I don't know if this grading is actually useful in practice, or if it just complicates SwiftLint's conceptual model as you say. I'd like to know more about how people use SwiftLint before acting on that though. We've reduced the number of violation severities before (#114), and I'm not opposed to abolishing it entirely if that's what active SwiftLint users suggest. |
|
@jpsim In my opinion both have their place - an alternative would be to allow consumers of SwiftLint to choose their own violation level for each provided rule? |
|
Yeah, I'd love support for violation levels for all rules, not just parameterized ones. |
I agree. It's nice that people can configure it in a way that fits their usage. Maybe a person working on a one-person team likes warnings, and is confident they will take the time to fix them, for instance. |
|
Though personally, I would not want to use SwiftLint that way. |
FWIW this can be done today but it's super ugly. type_body_length:
- 1000000000 # warning, never triggered
- 400 # errorThanks for sharing your thoughts on this. |
Can you do something like this for binary rules like the force cast rule? |
|
No, because it doesn't conform to |
Ah I see - interesting. How would you feel about us building functionality to allow users to change violation levels on all rules? |
I'd welcome that change, but I'd encourage you to propose a design for this first before implementing it because it'd be easy for such a change to introduce avoidable complexity. |
|
Thanks for your thoughts and quick responses, JP. |
|
@tamarnachmany did you give this any further thought? |
|
I actually just completed my 'v1' integration of SwiftLint! Very excited. It took a little bit of maneuvering because I want to lint gradually. I can now start taking a look at this. As far as this original issue I raised here, I'm not sure SwiftLint is working exactly as intended at the moment.
So the branch will not compile (good) but doesn't return a nonzero exit code (bad) which introduces an associated error ("Command /bin/sh emitted errors but did not return a nonzero exit code to indicate failure"). As far as I understand, the --strict flag should be returning a nonzero exit code if violations exist. Am I right in thinking this suggests an issue with Commandant/SwiftLint in this case? I distantly remember this working correctly previously? |
This is expected
These look like either bugs in SwiftLint, or an incomplete SwiftLint installation. The last message (SIGABRT) should be accompanied by a backtrace. Could you please share that here? In any case, I don't think any of this relates to the intended behavior of SwiftLint but rather a bug or misconfiguration, so I'd encourage you to file a new issue describing that so we can keep this thread focused on evaluating different |
|
Sure. |
|
Real quick before I repost this as a separate issue, is this what you're looking for? Is this what you're looking for? |
This is being printed because the return status of swiftlint is being ignored by your run script build phase. For example, if you had swiftlint
exit 0 # <- ignores the exit status from the swiftlint call aboveCan you please paste the output of your build phase? Using the script from the README shouldn't cause this problem. |
|
I don't see the |
|
The build phase doesn't have any output (unless I'm misunderstanding what you mean by output?). |
It sure sounds like you're not saving the exit code from each swiftlint invocation to then return with a non-zero code if an error was found. If you share the entirety of your build phase, without paraphrasing, I can help you refactor it to make the script exit with a non-zero code if any swiftlint invocation did.
All build script phases generate some output, usually this is available in Xcode:
|
You're right! I'm not. I thought that is a built-in functionality that doesn't need to be handled by a script. Does a script that runs linting have to handle that exit code? As far as output:
I am using the following build script (I'm in the process of modifying it so it does not have the correct version hard coded): |
Yes, although this is not specific to linting. This is how sh and bash work. I'm away from my computer now, but it should be trivial to adapt your script to return a code based on the linting results. |
|
TIL! I think I can take it from here. |
|
@tamarnachmany what's your take-away from this long thread? |
|
Hi. |
|
I mean what would you like to see changed in how SwiftLint deals with the |
|
To me it feels like the biggest question to tackle here is whether to support the configuration of violation severities at global scope, rule scope, or both. At rule scope, we can create a new protocol, say At a global scope, @jpsim 's idea of a But should we do both?? I'm not sure about that just because the potential ambiguities for end users creating or reading configurations. |
|
To me, doing rule-scope only seems the most practical. It will provide the desired behavior (though potentially more onerous configuration), it fits with what we are already doing with |
|
I also appreciate the current implementation of |
|
I enthusiastically agree with everything you suggested, Scott.
&
I think that's the way to go. And I think a protocol, and perhaps a new section in the .yml file with 'error-level-violations' and 'warning-level-violations' might be a nice structure there. That's without me actually trying to implement this yet. But from my exploration of the library, that seems good. As far as the --strict flag goes...hm... In the interim (before changes are potentially made to the way rule violations can be configured and are displayed) I do think more documentation on how to exit a build when style rules are violated would be great, and is probably relevant to many teams using SwiftLint. Looking at SwiftLint initially it seemed like maybe SwiftLint hooked back into some part of the Swift compilation process/ sent errors to SourceKit somewhere to report violations, rather than just printing them. Maybe a sentence in the readme, or a small sample script in the same way there is a sample .yml in this repo. I'm happy to contribute mine. |
|
@tamarnachmany , with the changes introduced in #391, you should now be able to configure the severity of any rule. Does this get you close enough to satisfy your use case? |
|
🙏 🙏 🙏 I will follow up with you/on the repo if I see any issues. |
|
I think it's fair time to close this. Let's reopen a new issue if anyone ever wants to revisit this. Thanks for spurring such a thorough conversation, @tamarnachmany! |
|
It is possible to treat all swiftlint warnings as errors by modifying the swiftlint output like this: Or in case you are using swiftlint from cocoapods:
|
|
Don't these suggested commands swallow the exit code from I'm no bash expert, but here's the script I ended up using: OUTPUT=$("${PODS_ROOT}"/SwiftLint/swiftlint --strict)
CODE=$?
echo $OUTPUT | sed 's/warning:/error:/g'
exit $CODE |
|
@bjtitus did you use |
|
@acecilia 🤦♂️ Thanks! Turns out I can't read. |
It seems that the purpose of the --strict flag in SwiftLint is to display or log linter violations as errors. I'd like to use that functionality but I am still seeing everything that has a
.Warninglevel of severity displayed in Xcode or logged on the command line as a warning.I have attempted to add this flag to a script which runs linting on certain project files, and also to linting on the command line.
In our build script I've tried:
fileis a variable in our build script.On the command line I've tried the same thing, but without a reference to any particular file, and with different file path specification syntax, since I don't believe SRCROOT works correctly there.
The build script displays these violations as warnings and the command on the command line prints "warning........" for each violation.
Has anyone else seen this issue?
Thanks!
The text was updated successfully, but these errors were encountered: