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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Force Unwrapping Rule to SwiftLint #1749

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

activcoding
Copy link
Member

Description

In PR #1673, we agreed on adding a force unwrapping rule to SwiftLint to improve our code quality across the project. Although this addition is not directly related to the original request in #1673, implementing this rule will help us write cleaner and safer code.

In this PR, I have:

  • Added the force unwrapping rule to SwiftLint.
  • Addressed a number of linter warnings by either fixing the issues or disabling the rule where appropriate and safe to do so.

If you come across a file you have worked on or will be working on, please take a moment to fix any linter warnings you encounter. This doesn't need to be done immediately, but can be addressed gradually over time.

Related Issues

Checklist

  • I read and understood the contributing guide as well as the code of conduct
  • The issues this PR addresses are related to each other
  • My changes generate no new warnings - It generates a few new warnings 馃槄
  • My code builds and runs on my machine
  • My changes are all related to the related issue above
  • I documented my code

Screenshots

@matthijseikelenboom
Copy link
Contributor

Should this also include a fix for this one:

image

@activcoding
Copy link
Member Author

As far as I know, this is in a separate repository. Abe has already fixed the issue, but we need to create another release.

@activcoding
Copy link
Member Author

I can do a release and update the version in CE first thing tomorrow morning.

FastestMolasses
FastestMolasses previously approved these changes Jun 7, 2024
@FastestMolasses
Copy link
Contributor

Looks like there's a ton of new lint errors from force unwrapping. Is it possible to write some custom rules so that we don't have to disable the linter for specific lines? For example in cases like this where we know a value already exists:

private var abc = URL("")!

@matthijseikelenboom
Copy link
Contributor

There are still some errors regarding linting

@activcoding
Copy link
Member Author

I left the force unwraps in place because I wasn't certain if removing them would break the code. Additionally, I avoided using swiftlint:disable as it might lead to forgetting to update the code later. If someone encounters a force unwrap warning in the file, it would be beneficial to reassess whether force unwrapping is truly necessary.

@matthijseikelenboom
Copy link
Contributor

How do we want to proceed here then? Do we merge this and fix it later? I don't think that is a good idea, because then we'd have to check what SwiftLint logs exactly to before we merge (Because it will say the check failed)

@activcoding
Copy link
Member Author

I've added swiftlint:disable:next where necessary, sometimes multiple times within a file. This ensures that we don't overlook it.

@activcoding
Copy link
Member Author

As for the CI failures, I'm not sure about those warnings. I can't see them on my local machine.

@matthijseikelenboom
Copy link
Contributor

@activcoding I'll try to take a look tonight

@austincondiff
Copy link
Collaborator

Do we have plans to remove these swiftlint:disable:next comments and fix each situation so it no longer requires force unwrapping in this PR? That is a lot of disable comments, are we okay with this?

Copy link
Collaborator

@0xWDG 0xWDG left a comment

Choose a reason for hiding this comment

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

IMHO there are to much swiftlint:disable comments, this should be reduced to the bare minimum required.

Comment on lines 29 to 32
if let bundlePath = Bundle.main.path(forResource: "Package", ofType: "resolved") {
let jsonData = try String(contentsOfFile: bundlePath).data(using: .utf8)
// swiftlint:disable:next force_unwrapping
let parsedJSON = try JSONDecoder().decode(AcknowledgementObject.self, from: jsonData!)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be a guard let, or if let.
There is no need to force unwap this.

Suggested change
if let bundlePath = Bundle.main.path(forResource: "Package", ofType: "resolved") {
let jsonData = try String(contentsOfFile: bundlePath).data(using: .utf8)
// swiftlint:disable:next force_unwrapping
let parsedJSON = try JSONDecoder().decode(AcknowledgementObject.self, from: jsonData!)
if let bundlePath = Bundle.main.path(forResource: "Package", ofType: "resolved"),
let jsonData = try String(contentsOfFile: bundlePath).data(using: .utf8) {
let parsedJSON = try JSONDecoder().decode(AcknowledgementObject.self, from: jsonData)

@@ -131,6 +131,7 @@ class EditorManager: ObservableObject {
if activeEditorHistory.isEmpty {
activeEditor = findSomeEditor(excluding: editor)
} else {
// swiftlint:disable:next force_unwrapping
activeEditor = activeEditorHistory.removeFirst()()!
Copy link
Collaborator

Choose a reason for hiding this comment

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

is ()() correct?

Comment on lines 19 to 20
let window = NSApp.windows.first { $0.identifier?.rawValue == SceneID.settings.rawValue }!
window.titlebarAppearsTransparent = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't something like this work?

Suggested change
let window = NSApp.windows.first { $0.identifier?.rawValue == SceneID.settings.rawValue }!
window.titlebarAppearsTransparent = true
NSApp.windows.first { $0.identifier?.rawValue == SceneID.settings.rawValue }?.titlebarAppearsTransparent = true

@activcoding
Copy link
Member Author

@austincondiff, @0xWDG

I've added swiftlint:disable:next where necessary, sometimes multiple times within a file. This ensures that we don't overlook it.

In my experience, without adding swiftlint:disable to every force unwrap, no one will bother to remove them. For the proposed changes, could you open a PR and fix some instances of force unwrapping?

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.

馃Ч Improvements to SwiftLint
5 participants