-
Notifications
You must be signed in to change notification settings - Fork 338
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
chore: Enable force_unwrapping for SwiftLint #4820
Conversation
1 build decreased size
Paywalls 1.0 (1)
|
Item | Install Size Change |
---|---|
Other | ⬇️ -494 B |
🛸 Powered by Emerge Tools
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this! I agree that we should avoid force unwraps in general, so it's better to add the rule to the linter 👌
Tests/StoreKitUnitTests/StoreKit2/StoreKit2TransactionListenerTests.swift
Outdated
Show resolved
Hide resolved
I'd like to offer a counter point: most of the diff in this PR is adding (I'm not blocking this PR, just wanted to share some food for thought.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Regarding @JayShortway's point, IMO, I think this is a good rule to have, since we should normally avoid force-unwrapping. I think the comments right now are added so we minify the changes in the existing code, but we probably should address those as well. At least, enabling the rule would make us avoid force-unwrapping if possible in future code.
@tonidero Alright, yea makes sense. I don't feel as strongly about the rule (we're all aware that we should be careful when force unwrapping), but as I said it's not a blocker. Seems that SwiftLint has a |
@JayShortway as @tonidero said, I think it's good to have, and it's good to disable it as well when needed. Mainly because relying to code reviews to avoid potential crashes due to force unwraps is unnecessary IMO. If we disable it intentionally, it will also make the reviewer think twice to see if the disabling makes sense :) I added a bunch of disables because I lack a lot of context in some parts of the code, but all in to remove them gradually. I have no idea what baseline is, but it'll take a look |
@facumenzella I can definitely agree to the better-safe-than-sorry mindset! Regarding baseline, it's basically a file containing the current violations in the code, with the intent of fixing them later. It's a useful feature when introducing a new linter or rule. The new rule(s) will immediately apply for all future code going forward, while not requiring you to fix (or ignore) existing violations. Detekt and Android Lint have it too. For reference, here's our Detekt baseline in purchases-android. |
Motivation
A couple of days ago I noticed #4794
Description
Enable
force_unwrapping
rule for SwiftLint. There's no reason to not have this enabled. It can always be disabled manually when needed. Better safe than sorry :)