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

tonell-m · 2025-01-08T07:37:30Z

📜 Tickets

💡 Description

Now that all implicitly_unwrapped_optional violations in source files are resolved (still waiting on #23809 and #23880 to be merged), enable the rule in the Swiftlint configuration.

Since most of the test files use implicitly unwrapped optional properties that are initialized in setUp and reset in tearDown, we would like not to enable this rule for test files. An approach is to use Swiftlint nested configurations by adding additional .swiftlint.yml files that can alter the global configuration for subfolders (in this case, disable implicitly_unwrapped_optional for Tests folders).

Nested configurations cannot be used together with the --config option to point to the Swiftlint config file, so the workaround is to navigate to the directory containing the config before running swiftlint in the build phase.

📝 Checklist

You have to check all boxes before merging

Filled in the above information (tickets numbers and description of your work)
Updated the PR name to follow our PR naming guidelines
Wrote unit tests and/or ensured the tests suite is passing
When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
If needed, I updated documentation / comments for complex code and public methods
If needed, added a backport comment (example @Mergifyio backport release/v120)

…ictions

FilippoZazzeroni

It looks great to me this is a really nice catch 🎉 🎉 . tagging also @OrlaM and @adudenamedruby to get extra eye on this modification since it impact a bit the project structure.
If this modification lands in i'll add it to our wiki.

FilippoZazzeroni · 2025-01-08T10:53:06Z

firefox-ios/Client.xcodeproj/project.pbxproj

@@ -15861,7 +15861,7 @@
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 			shellPath = /bin/sh;
-			shellScript = "if [[ \"$(uname -m)\" == arm64 ]]; then\n    export PATH=\"/opt/homebrew/bin:$PATH\"\nfi\n\nMODIFIED_FILES=$(git diff --name-only --diff-filter=ACM | grep -e '\\.swift$')\n\nif which swiftlint > /dev/null; then\n    for file in $MODIFIED_FILES; do\n        swiftlint lint \"${SRCROOT}/../${file}\" --config \"${SRCROOT}/../.swiftlint.yml\"\n    done\nelse\n    echo \"warning: SwiftLint not installed, download from https://github.com/realm/SwiftLint\" \nfi\n";
+			shellScript = "if [[ \"$(uname -m)\" == arm64 ]]; then\n    export PATH=\"/opt/homebrew/bin:$PATH\"\nfi\n\nMODIFIED_FILES=$(git diff --name-only --diff-filter=ACM | grep -e '\\.swift$')\n\n# Location of the folder where the root swiftlint config is located.\n# Swiftlint commands will be ran from this location, allowing us to make use\n# of swiftlint's nested configurations to tweak rules for certain source\n# sets (e.g. test files).\n# Nested configuration can not work together with --config option, so we need\n# to run the swiftlint command from the directory containing the root\n# configuration file.\n# https://github.com/realm/SwiftLint#nested-configurations\nSWIFTLINT_ROOT=\"${SRCROOT}/../\"\n\nif which swiftlint > /dev/null; then\n    for file in $MODIFIED_FILES; do\n        ( cd ${SWIFTLINT_ROOT} && swiftlint lint \"${SRCROOT}/../${file}\" )\n    done\nelse\n    echo \"warning: SwiftLint not installed, download from https://github.com/realm/SwiftLint\" \nfi\n";


Just a small change the rest looks great 🎉 . i'd have:

// move out of the for loop cd $SWIFTLINT_ROOT for loop {}

I'd also reduce the comment a bit and add it directly to the cd $SWIFTLINT_ROOT saying that this instruction is needed here in order to have nested swiftlint configurations.

Good point, done!

lmarceau · 2025-01-08T14:53:01Z

BrowserKit/Tests/.swiftlint.yml

+# nested configuration takes over.
+# https://github.com/realm/SwiftLint#nested-configurations.
+
+disabled_rules:


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

FilippoZazzeroni

It looks great to me. Thanks again for your contribution 🎉 🎉

tonell-m · 2025-01-09T08:50:52Z

Hi @FilippoZazzeroni, from the looks of the Bitrise output there are still some violations in SampleBrowser and SampleComponentLibraryApp, I'll open some PRs for those.

However, it seems that the Bitrise Swiftlint phase script still uses the --config option so it also lints the test files - I'm not sure I can fix this myself

adudenamedruby · 2025-01-09T14:33:28Z

LGTM :). I'll let @FilippoZazzeroni push this through

FilippoZazzeroni · 2025-01-09T17:23:48Z

Hi @tonell-m sorry for the late response, i've been going deeper into your solution and i see some nits that i'd like to understand better.
Right now we have two main ways of running swiftlint and those are via the run script and via CLI in this case via Bitrise.
The problem i see right now is that by running swiftlint even with the --config via the run script on firefox, it can create warnings of files out of the firefox project, like for example in SampleBrowser.
Also i noticed that only for firefox-ios we have a root yml file meanwhile for the other projects like Focus and SampleComponentAppLibrary we have a project level config yml.
I need to explore a bit more this solution and check with the team if we can move the firefox ios yml to its project folder so we keep it isolated like the other projects and we can then run directly swiftlint in the project folder.

I'll look better into it tomorrow and let you know as soon as i have news.
In the meantime if you need any help on anything we can chat directly on Element chat

tonell-m · 2025-01-10T08:40:09Z

Hi @FilippoZazzeroni! I do agree with the points you're raising, personally (with the knowledge I have atm) I would suggest the following:

When we run Swiftlint from the build phase of either firefox-ios or focus-ios we should add a mask to the files that will be linted. Currently the linted files are obtained via git diff, but we could imagine adding folders to that command like so
```
git diff --name-only --diff-filter=ACM ${BROWSER_KIT_ROOT} ${SRC_ROOT} | grep -e '\.swift$'
```
That would limit the linted files to the ones tied to the current project + BrowserKit since it is shared between both apps
Same goes for linting in Bitrise, we could pass only the sources involved in the current project like swiftlint BrowserKit/**/*.swift firefox-ios/**/*.swift or swiftlint BrowserKit/**/*.swift focus-ios/**/*.swift
As for the configuration, it would make sense to me to have a shared swiftlint config at root level to unify linter rules accross firefox-ios and focus-ios but with nested configs we could also have specific configurations for both projects to enable/disable specific rules if needed

Let me know what you think when you'll have discussed it with your team ✌️

FilippoZazzeroni · 2025-01-10T09:14:05Z

Thanks very much @tonell-m i like this idea, i'll come up with a proposal and let you know how we can proceed.

tonell-m added 3 commits January 7, 2025 13:35

Enable implicitly_unwrapped_optional rule in swiftlint configuration

ffceb00

Disable rule for test files using swiftlint nested configuration

15ec372

Update Swiftlint run phase to comply with nested configurations restr…

002cefb

…ictions

tonell-m requested a review from a team as a code owner January 8, 2025 07:37

tonell-m requested a review from mdotb-moz January 8, 2025 07:37

FilippoZazzeroni reviewed Jan 8, 2025

View reviewed changes

FilippoZazzeroni requested review from OrlaM and adudenamedruby and removed request for mdotb-moz January 8, 2025 10:56

Move cd outside of for loop and reduce comment size

7feedec

lmarceau reviewed Jan 8, 2025

View reviewed changes

FilippoZazzeroni approved these changes Jan 8, 2025

View reviewed changes

adudenamedruby removed their request for review January 9, 2025 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tonell-m commented Jan 8, 2025

FilippoZazzeroni left a comment

FilippoZazzeroni Jan 8, 2025 •

edited

Loading

tonell-m Jan 8, 2025

lmarceau Jan 8, 2025

FilippoZazzeroni left a comment

tonell-m commented Jan 9, 2025 •

edited

Loading

adudenamedruby commented Jan 9, 2025

FilippoZazzeroni commented Jan 9, 2025

tonell-m commented Jan 10, 2025

FilippoZazzeroni commented Jan 10, 2025

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

Are you sure you want to change the base?

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

Conversation

tonell-m commented Jan 8, 2025

📜 Tickets

💡 Description

📝 Checklist

FilippoZazzeroni left a comment

Choose a reason for hiding this comment

FilippoZazzeroni Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

tonell-m Jan 8, 2025

Choose a reason for hiding this comment

lmarceau Jan 8, 2025

Choose a reason for hiding this comment

FilippoZazzeroni left a comment

Choose a reason for hiding this comment

tonell-m commented Jan 9, 2025 • edited Loading

adudenamedruby commented Jan 9, 2025

FilippoZazzeroni commented Jan 9, 2025

tonell-m commented Jan 10, 2025

FilippoZazzeroni commented Jan 10, 2025

FilippoZazzeroni Jan 8, 2025 •

edited

Loading

tonell-m commented Jan 9, 2025 •

edited

Loading