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

Adjust --strict to treat warnings as errors instead of only altering exit code #3372

Merged
merged 3 commits into from
Nov 7, 2020

Conversation

Jeehut
Copy link
Collaborator

@Jeehut Jeehut commented Oct 3, 2020

Implements #3312 with @jpsim's suggested solution.

I tested that this works manually by building via swift build -c release and running .build/release/swiftlint --strict and it worked just as expected. Without that option, I had 1 warning & 1 error reported, with that options 2 errors were reported, so I can confirm it works. But I did not add any unit tests because there seem to be no unit tests for the CLI product, other options like --lenient seem also not to be unit tested.

Note that I did not mess with any tresholds (like lenient does), I think we could think about those things after seeing how people are using this new option, so I'd rather keep this simple for now.

Also note that I have decided to put the Changelog entry into the "Breaking Changes" section as some people might have been relying on the fact that using --strict was still making a distinction between warnings & errors when using the output of reporters directly. As this is changing this documented behavior, I feel like it would be wrong to place the change into the "Bug Fixes" section. But I can also move it there if needed, I do not have a strong opinion here.

@Jeehut Jeehut changed the title Adjust strict mode to treat warnings as errors instead of only altering exit code Adjust --strict to treat warnings as errors instead of only altering exit code Oct 3, 2020
@SwiftLintBot
Copy link

1 Warning
⚠️ This PR may need tests.
12 Messages
📖 Linting Aerial with this PR took 1.94s vs 1.91s on master (1% slower)
📖 Linting Alamofire with this PR took 2.72s vs 2.7s on master (0% slower)
📖 Linting Firefox with this PR took 9.13s vs 9.07s on master (0% slower)
📖 Linting Kickstarter with this PR took 15.3s vs 15.22s on master (0% slower)
📖 Linting Moya with this PR took 1.3s vs 1.28s on master (1% slower)
📖 Linting Nimble with this PR took 1.36s vs 1.4s on master (2% faster)
📖 Linting Quick with this PR took 0.61s vs 0.66s on master (7% faster)
📖 Linting Realm with this PR took 2.64s vs 2.65s on master (0% faster)
📖 Linting SourceKitten with this PR took 1.03s vs 1.05s on master (1% faster)
📖 Linting Sourcery with this PR took 7.05s vs 7.1s on master (0% faster)
📖 Linting Swift with this PR took 10.87s vs 10.9s on master (0% faster)
📖 Linting WordPress with this PR took 16.96s vs 16.98s on master (0% faster)

Generated by 🚫 Danger

@codecov-commenter
Copy link

codecov-commenter commented Oct 3, 2020

Codecov Report

Merging #3372 into master will increase coverage by 0.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3372      +/-   ##
==========================================
+ Coverage   90.48%   90.52%   +0.03%     
==========================================
  Files         417      417              
  Lines       20508    20512       +4     
==========================================
+ Hits        18557    18568      +11     
+ Misses       1951     1944       -7     
Impacted Files Coverage Δ
Source/swiftlint/Commands/AnalyzeCommand.swift 0.00% <0.00%> (ø)
Source/swiftlint/Commands/LintCommand.swift 0.00% <0.00%> (ø)
...ource/swiftlint/Helpers/LintOrAnalyzeCommand.swift 0.00% <0.00%> (ø)
...tyle/MultipleClosuresWithTrailingClosureRule.swift 85.52% <0.00%> (+1.31%) ⬆️
...mework/Rules/Idiomatic/NoFallthroughOnlyRule.swift 100.00% <0.00%> (+1.92%) ⬆️
...ramework/Rules/Style/NoSpaceInMethodCallRule.swift 100.00% <0.00%> (+15.78%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da408b5...fa7380c. Read the comment docs.

@Jeehut Jeehut requested review from jpsim and marcelofabri November 7, 2020 19:54
@jpsim
Copy link
Collaborator

jpsim commented Nov 7, 2020

But I did not add any unit tests because there seem to be no unit tests for the CLI product, other options like --lenient seem also not to be unit tested.

Correct, this is unfortunate, and ideally I'd like to get rid of our pbxproj Xcode project altogether, which would then make it easier to add a test target just using the SwiftPM manifest that can test the swiftlint executable product.

@jpsim
Copy link
Collaborator

jpsim commented Nov 7, 2020

Also note that I have decided to put the Changelog entry into the "Breaking Changes" section

Thanks, I think that's the right call too!

Copy link
Collaborator

@jpsim jpsim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, I probably should have done it this way originally. Thanks!

}

case (true, true):
queuedFatalError("Invalid command line options: 'lenient' and 'strict' are mutually exclusive.")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

@jpsim jpsim merged commit 8194244 into master Nov 7, 2020
@jpsim jpsim deleted the cg-fix-strict-mode branch November 7, 2020 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants