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

Refactor FXIOS-10205 [Swiftlint] Enable implicitly_unwrapped_optional rule but keep it disabled for test files using nested Swiftlint configurations #24031

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .swiftlint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ only_rules: # Only enforce these rules, ignore all others
- accessibility_trait_for_button
- force_cast
- closure_body_length
- implicitly_unwrapped_optional

# These rules were originally opted into. Disabling for now to get
# Swiftlint up and running.
Expand Down
9 changes: 9 additions & 0 deletions BrowserKit/Tests/.swiftlint.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# Swiftlint configuration overloads that will be applied to all files
# in this folder and all its children recursively, unless another
# nested configuration takes over.
# https://github.com/realm/SwiftLint#nested-configurations.

disabled_rules:
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks great! Thanks for the work on this issue 🙌 I'll let my colleagues do the final review!

# Many test use implicitly unwrapped optional properties that
# initialized in setUp and reset in tearDown.
- implicitly_unwrapped_optional
8 changes: 4 additions & 4 deletions SampleBrowser/SampleBrowser/Engine/EngineProvider.swift
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@ import WebEngine

struct EngineProvider {
// We only have one session in the SampleBrowser
private(set) var session: EngineSession?
private(set) var session: EngineSession
let view: EngineView

init(engine: Engine = WKEngine.factory(),
sessionDependencies: EngineSessionDependencies? = nil) {
init?(engine: Engine = WKEngine.factory(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if this is intended behavior, LGTM just tagging @lmarceau that worked on this implementation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wow, fast review! As I've mentioned below:

SampleBrowser doesn't compile anymore because RootViewController is missing a method from AddressToolbarDelegate

So I couldn't really test this fix, but it was the simplest workaround to get rid of the implicitly unwrapped property. Since engineSession was implicitly unwrapped in BrowserViewController it would have crashed anyway if engineProvider.session was nil

Copy link
Contributor

Choose a reason for hiding this comment

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

The change works for me 👍

sessionDependencies: EngineSessionDependencies? = nil) {
do {
session = try engine.createSession(dependencies: sessionDependencies)
} catch {
session = nil
return nil
}

view = engine.createView()
Expand Down
4 changes: 2 additions & 2 deletions SampleBrowser/SampleBrowser/SceneDelegate.swift
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,15 @@ import WebEngine

class SceneDelegate: UIResponder, UIWindowSceneDelegate {
var window: UIWindow?
var engineProvider: EngineProvider = {
var engineProvider: EngineProvider? = {
let dependencies = EngineSessionDependencies(telemetryProxy: TelemetryHandler())
return EngineProvider(sessionDependencies: dependencies)
}()

func scene(_ scene: UIScene,
willConnectTo session: UISceneSession,
options connectionOptions: UIScene.ConnectionOptions) {
guard let windowScene = (scene as? UIWindowScene) else { return }
guard let windowScene = (scene as? UIWindowScene), let engineProvider else { return }
let windowUUID = UUID()
let baseViewController = RootViewController(engineProvider: engineProvider, windowUUID: windowUUID)
window = UIWindow(windowScene: windowScene)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class BrowserViewController: UIViewController,
FindInPageHelperDelegate {
weak var navigationDelegate: NavigationDelegate?
private lazy var progressView: UIProgressView = .build { _ in }
private var engineSession: EngineSession!
private var engineSession: EngineSession
private var engineView: EngineView
private let urlFormatter: URLFormatter

Expand Down Expand Up @@ -203,7 +203,7 @@ class BrowserViewController: UIViewController,
guard let url = linkURL else { return nil }

let previewProvider: UIContextMenuContentPreviewProvider = {
let previewEngineProvider = EngineProvider()
guard let previewEngineProvider = EngineProvider() else { return nil }
let previewVC = BrowserViewController(engineProvider: previewEngineProvider)
previewVC.engineSession.load(url: url.absoluteString)
return previewVC
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ protocol SuggestionViewControllerDelegate: AnyObject {

class SuggestionViewController: UIViewController, UITableViewDelegate {
private var tableView: UITableView
private var dataSource: SuggestionDataSource!
private var dataSource: SuggestionDataSource?
private weak var delegate: SuggestionViewControllerDelegate?

private var gradientLayer: CAGradientLayer?
Expand Down Expand Up @@ -87,14 +87,14 @@ class SuggestionViewController: UIViewController, UITableViewDelegate {

func updateUI(for suggestions: [String]) {
tableView.isHidden = suggestions.isEmpty
dataSource.suggestions = suggestions
dataSource?.suggestions = suggestions
tableView.reloadData()
}

// MARK: - UITableViewDelegate

func tableView(_ tableView: UITableView, didSelectRowAt indexPath: IndexPath) {
let term = dataSource.suggestions[indexPath.row]
guard let term = dataSource?.suggestions[indexPath.row] else { return }
delegate?.tapOnSuggestion(term: term)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@ class BottomSheetChildViewController: UIViewController, BottomSheetChild, Themea
label.numberOfLines = 0
}

private var heightConstraint: NSLayoutConstraint!

init(themeManager: ThemeManager = AppContainer.shared.resolve()) {
self.themeManager = themeManager
super.init(nibName: nil, bundle: nil)
Expand Down
9 changes: 9 additions & 0 deletions firefox-ios/firefox-ios-tests/Tests/.swiftlint.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# Swiftlint configuration overloads that will be applied to all files
# in this folder and all its children recursively, unless another
# nested configuration takes over.
# https://github.com/realm/SwiftLint#nested-configurations.

disabled_rules:
# Many test use implicitly unwrapped optional properties that
# initialized in setUp and reset in tearDown.
- implicitly_unwrapped_optional
1 change: 1 addition & 0 deletions focus-ios/.swiftlint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,5 @@ disabled_rules:
- attributes
- trailing_whitespace
- vertical_whitespace
- implicitly_unwrapped_optional