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

chore: Enable force_unwrapping for SwiftLint #4820

Merged
merged 5 commits into from
Feb 26, 2025
Merged

Conversation

facumenzella
Copy link
Contributor

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 :)

Copy link

emerge-tools bot commented Feb 25, 2025

1 build decreased size

Name Version Download Change Install Change Approval
Paywalls
com.revenuecat.PaywallsTester
1.0 (1) 11.2 MB ⬆️ 722 B 42.3 MB ⬇️ 494 B N/A

Paywalls 1.0 (1)
com.revenuecat.PaywallsTester

⚖️ Compare build
📦 Install build
⏱️ Analyze build performance

Total install size change: ⬇️ 494 B
Total download size change: ⬆️ 722 B

Largest size changes

Item Install Size Change
Other ⬇️ -494 B
View Treemap

Image of diff


🛸 Powered by Emerge Tools

Copy link
Member

@ajpallares ajpallares left a 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 👌

@JayShortway
Copy link
Member

I'd like to offer a counter point: most of the diff in this PR is adding // swiftlint:disable force_unwrapping. Is this rule really necessary? Rules like this can add a lot of friction, and I'm wondering if it's worth it.

(I'm not blocking this PR, just wanted to share some food for thought.)

Copy link
Contributor

@tonidero tonidero left a 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.

@JayShortway
Copy link
Member

@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 baseline feature, but it's not really documented. (To be clear, I'm not suggesting to add that. 🙂)

@facumenzella
Copy link
Contributor Author

@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

@JayShortway
Copy link
Member

@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.

@facumenzella facumenzella enabled auto-merge (squash) February 26, 2025 11:24
@facumenzella facumenzella enabled auto-merge (squash) February 26, 2025 11:25
@facumenzella facumenzella merged commit 6b5c430 into main Feb 26, 2025
10 checks passed
@facumenzella facumenzella deleted the fix/force_unwrapping branch February 26, 2025 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants