Refactor how we handle .swiftlint.yml

[data-sync-user]

📄 Description

Starting from this great PR that introduces the concept of nested .swiftlint.yml we would like to refactor our monorepo to introduce this concept. Some steps are needed in order to introduce this change since the repo at the moment is not consistent on how it handles .swiftlint.yml across all the projects.

The current state of the monorepo is the following:

• Firefox: uses in its run script the .swiftlint.yml at the root of the repo
• Focus: uses focus-ios/.swiftlint.yml in its run script that basically is a copy of the root one with less rules and different excluded path
• SampleComponentLibraryApp: Has lint run script but with no custom .swiftlint.yml
• SampleBrowser: Has no lint run script

This first task is to merge the focus/.swiftlint.yml into the root .swiftlint.yml and then adjust the Focus and Firefox lint run script to be run from the root of the project.

Since Focus and Firefox share almost the same rules, create a nested .swiftlint.yml for focus that holds the rules that are missing and disable them so to have the same behavior of before.

We’d like to have only one main .swiftlint.yml at the root of the repo so to have all the shared rules there and optionally customize other rules in the nested config files.

Make sure when merging focus config files that the excluded folders are ported into the root one.

ℹ️ Reference Person

@FilippoZazzeroni

┆Issue is synchronized with this Jira Task

[tonell-m]

Hi @FilippoZazzeroni! Can you assign this ticket to me please?

[FilippoZazzeroni]

Thank you @lmarceau 😃

[tonell-m]

Hey @FilippoZazzeroni, I did a bit of research yesterday night about this and ran into some stuff I would like to discuss with you before continuing. First of all, there was a bit of a misunderstanding about what is possible to do with nested configs:

In other words: Nested configurations don't work recursively – there's a maximum number of one nested configuration per file that may be applied in addition to the main configuration.

So basically the structure that we were thinking of would not actually work (or at least not as we expected it would):

|-- .swiftlint.yml
|-- firefox-ios/
    |-- .swiftlint.yml
    |-- Sources/
        |-- SomeSource.swift.   <- Will use root config + firefox-ios/.swiftlint.yml
    |-- Tests/
        |-- .swiftlint.yml
        |-- SomeTest.swift      <- Will use root config + firefox-ios/Tests/.swiftlint.yml but will ignore firefox-ios/.swiftlint.yml

However, since Bitrise was linting all files using the root swiftlint config (--config option is actually enabled by default if you don't specify an empty config path in the Bitrise step that is in use atm), it turns out all sources under focus-ios already comply to the same set of rules that are used for firefox-ios. Meaning if I run swiftlint --config .swiftlint.yml focus-ios/**/*.swift it will raise no violations (not even implicitly_unwrapped_optional, how lucky 😁).

---

Taking all of that into account, the best option I can think of would be to simply get rid of focus-ios/.swiftlint.yml (or just move some exclude paths to the root config but keep the rules from firefox-ios since they are the most strict between the two). Then only use nested configurations to disable rules in Tests directories, or other places where necessary, but keeping in mind that this can only be one level deep.

The downside to this approach would be that adding a new rule to the root configuration would mean having to enforce it in every subproject (but it kind of is already the case atm because of the current Bitrise config). If really needed, we could still have multi level configuration but the content of the parent config would need to be duplicated down into the children.

What is your stance on this? (Hope I managed to be clear in my explanations 😅)

[FilippoZazzeroni]

Ok thanks so much for the report @tonell-m i'm having a look but i guess we don't have much alternatives, so i'm taking a moment to reflect on what next, maybe we take this is as a chance to refactor the lint and use one common file for all the projects let see 😃

[tonell-m]

Great @FilippoZazzeroni, let me know when you've settled on a plan then 🙂

If it can help, what I can think of now is:

• Merge exclude paths of focus-ios/.swiftlint.yml into the root config file, then delete the sub config
• Update build phases of each sub projects to
  • Use nested configurations instead of --config (using cd ${SWIFTLINT_ROOT}
  • Adapt the lookup path of ${MODIFIED_FILES} to only look for files in the current project
• Update bitrise.yml to use nested configs instead of --config approach
• Add nested configurations in test folders (or maybe that should be done as part of Refactor FXIOS-10205 [Swiftlint] Enable implicitly_unwrapped_optional rule but keep it disabled for test files using nested Swiftlint configurations #24031 instead)

[FilippoZazzeroni]

Hi @tonell-m i did verify the one level down path and i cannot repro on my PC, if i set a nested yml file for firefox for example it disable the rule for the whole project, it seems to work as expected. Let me know if u can repro on your PC, also we can pair if you want 😃

[tonell-m]

Hey @FilippoZazzeroni, I've made a minimal example to demonstrate this when I was investigating the matter, I've pushed it in a public repository that you can check out: https://github.com/tonell-m/swiftlint-nested-config-test.

You'll see in this project that there can be at most two configurations in use per file: the root level configuration and the nested configuration that is "closest" to the file being linted. When trying to provide multiple configuration overloads on top of the root config, only the deepest one will be actually considered.

So in our case, that would mean if we have a root level configuration, then add an overload for the whole focus-ios subproject for instance, if we also want to add another configuration overload only for test files inside focus-ios/focus-ios-tests, the configuration overload in focus-ios will actually be bypassed.

Actually maybe there was a misunderstanding: the configuration overload does propagate recursively to all files in subdirectories, but the limitation is that if at some level another nested configuration is found, it will replace the one currently in use and not add the refined rules on top of it.

Let me know if that answers your questions 🙂

[FilippoZazzeroni]

Hi @tonell-m great example ok now i understand your point. Then i'd go to have one root config, and have one nested config for each project just for tests. Each project should be aligned with the root config. It also makes sense on my side since we have to maintain less configuration

[tonell-m]

Hi @FilippoZazzeroni I've just opened a PR for this. However I was mistaken when I said that the sources in focus-ios didn't trigger any linter violations, so I've still kept a configuration overload in focus-ios/.swiftlint.yml that only disable those rules. I'm guessing a next step could be to open issues to resolve violations for each of those rules so we can end up with only one configuration in the end.

Still that setup, even if not final, would already allow us to complete #24031 by adding nested configs for tests in firefox-ios and BrowserKit.