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

Report and Improvements on Swiftlint baseline #5597

Closed
2 tasks done
aiKrice opened this issue May 19, 2024 · 8 comments
Closed
2 tasks done

Report and Improvements on Swiftlint baseline #5597

aiKrice opened this issue May 19, 2024 · 8 comments

Comments

@aiKrice
Copy link

aiKrice commented May 19, 2024

New Issue Checklist

Describe the bug

I played this week end on with the baseline feature. First of all thank @mildm8nnered and all contributors, Amazing job and I am here also to help with my way, by trying hard.

I noticed several behavior that imho, are not exactly what I expected, as an Android developer as well, Detekt and AndroidLint baseline user.

1️⃣. I generate a baseline with this command:

$ swiftlint --write-baseline config/swiftlint/baseline.json

I noticed a different behavior in the content of the baseline if I specifiy my config file

$ swiftlint --write-baseline config/swiftlint/baseline.json --config config/swiftlint/swifltint.yml

2️⃣. In both case above, I noticed that some rules some not ignored in the baseline so swiftlint failed. It was these rules :

  • explicit_top_level_acl
  • explicit_acl
  • prefixed_toplevel_constant

It's maybe because this file was listed as excluded in my config file (see it below #githooks)

3️⃣. I generated a baseline with all rules enabled (as suggested in another issue opened) (but baseline should not be generated this way but against a configuration file), In this case "it works".

4️⃣. The baseline json file is a single 1 line file 😱 . In this case, Xcode (well, xcode...) cant read it. So I use a third party to format it: prettier -> https://github.com/prettier/prettier to format it. I think this should be opt-in because to compare this on a PR even to make it readable, it better. But by using prettier and an intermediate tool, there are some noises and too many illegitimates diffs.

Voila, hoping it can help, I am ready to kepp helping for improving it on my project 💪 .

Environment

version: 0.55.1
Homebrew, Mac M1PRO 14"
Xcode 15.4
Build version 15F31d

disabled_rules: # rule identifiers to exclude from running
  - accessibility_label_for_image
  - accessibility_trait_for_button
  - anonymous_argument_in_multiline_closure
  - attributes
  - balanced_xctest_lifecycle
  - closure_end_indentation
  - closure_spacing
  - closure_body_length
  - collection_alignment
  - conditional_returns_on_newline
  - discouraged_optional_boolean
  - discouraged_optional_collection
  - explicit_type_interface
  - function_body_length
  - ibinspectable_in_extension
  - indentation_width
  - let_var_whitespace
  - literal_expression_end_indentation
  - missing_docs
  - number_separator # Swiftformat
  - object_literal
  - operator_usage_whitespace
  - one_declaration_per_file
  - period_spacing
  - prefer_nimble
  - prohibited_interface_builder
  - raw_value_for_camel_cased_codable_enum #Need to be reactivated when we will refactor the model
  - required_deinit
  - switch_case_on_newline # Swiftformat
  - switch_case_alignment # Swiftformat
  - trailing_whitespace #SwiftFormat
  - unneeded_parentheses_in_closure_argument # Swiftformat
  - vertical_parameter_alignment_on_call # Swiftformat
  - vertical_whitespace_between_cases # Swiftformat
  - vertical_whitespace_closing_braces # Swiftformat
  - vertical_whitespace_opening_braces # Swiftformat
  - xct_specific_matcher

analyzer_rules:
  - capture_variable
  - explicit_self
  - typesafe_array_init
  - unused_declaration
  - unused_import
 
opt_in_rules: # some rules are only opt-in
  - all
  # Find all the available rules by running:
  # swiftlint rules
included: # paths to include during linting. `--path` is ignored if present.
  - Shopmium
  - Packages
excluded: # paths to ignore during linting. Takes precedence over `included`.
  - build
  - Shopmium/Resources
  - Shopmium/AppConfig/Constants.swift
  - Shopmium/BuildGenerated/Sourcery/Generated
  - Packages/Foundation/ShopmiumSDK # Baseline: Please remove this line when the SDK will be ready to be linted
  - Packages/Foundation/Alamofire
  - Packages/Foundation/SparrowKit/Sources/SparrowKit/Classes/Constants
  - ShopmiumBATGenerator
  - Shopmium/UI/Feature/Explorer
  - ShopmiumUITests
  - ShopmiumUnitTests
  - Showcase/Showcase
  - githooks # <------------------------ my file is inside this folder 
analyzer_rules: # Rules run by `swiftlint analyze` (experimental)
  - explicit_self

# configurable rules can be customized from this configuration file
# binary rules can set their severity level
force_cast: warning # implicitly
force_try:
  severity: warning # explicitly

file_name:
  excluded:
    - "AppearanceThemeDomainMapper.swift"
    - "BackgroundManager.swift"
    - "DeprecationAlert.swift"
    - "EventListenerContractExtension.swift"
    - "Factories.swift"
    - "FavoritesManager.swift"
    - "FavoritesRouter.swift"
    - "GenerateBarcode.swift"
    - "Loader.swift"
    - "LoginManager.swift"
    - "PiggyBankViewExtension.swift"
    - "OffersManager.swift"
    - "OffersListCellTypeExtension.swift"
    - "OffersTabUIItemTypeExtension.swift"
    - "OfferTagsLimitation.swift"
    - "OfferTagsLockedOrUnlocked.swift"
    - "OfferTagsRemainingCoupons.swift"
    - "OfferTagsShopmiumClub.swift"
    - "OfferTagsTimeStatus.swift"
    - "PaymentManager.swift"
    - "ReportManager.swift"
    - "SparrowAlias.swift"
    - "SubmissionsManager.swift"
    - "TabBarContentViews.swift"
    - "UIViewAnimations.swift"
    - "UserRepository.swift"
    - "UserStore.swift"
    - "WalletRouter.swift"
    - "XCAssets+Generated.swift"
    - "XibLoader.swift"


line_length: 120

type_name:
  min_length: 4 # only warning
  max_length: # warning and error
    warning: 40
    error: 50
  excluded: iPhone # excluded via string
  allowed_symbols: ["_"] # these are allowed in type names
reporter: "xcode" # reporter type (xcode, json, csv, checkstyle, junit, html, emoji, sonarqube, markdown)
@mildm8nnered
Copy link
Collaborator

Hi @aiKrice - I replied to your comment here - #5475 (comment), but I'll reply here as well.

Firstly, thank you so much for the feedback, which is incredibly useful (and also for the Medium articles - it is great to get the word out there).

Coming back to your specific issues:

  1. "I noticed a different behavior in the content of the baseline if I specify my config file"

You need to use the same config file when you generate the baseline with --write-baseline as when you use it for filtering via --baseline, as the config file will determine which rules are in effect, and also things like rule configurations, thresholds for individual rules etc.

This should work, as in both cases, swiftlint is just running the lint command, and this has hardly changed in the baseline PR, apart from some hooks to write and/or read the baseline.

  1. "In both case above, I noticed that some rules some not ignored in the baseline so swiftlint failed. It was these rules "

It's maybe because this file was listed as excluded in my config file" - it is exactly that.

  1. "I generated a baseline with all rules enabled (as suggested in another issue opened) (but baseline should not be generated this way but against a configuration file), In this case "it works"."

You can generate a baseline with --enable-all-rules, but you should not do that, as it will contain all possible violations, and will not have any rule configurations or rule specific thresholds etc.

The reason that is mentioned in #5553 is more as a way of saying - "these are all our possible violations, let's pick which rules from here we would like to enable".

  1. "The baseline json file is a single 1 line file "

I agree that this can be a bit annoying and unhelpful when trouble-shooting. During development, I did turn pretty printing on, and in the Periphery implementation of the baseline feature I currently have pretty-printing on.

The reason I turned this off was because of concerns about baseline file size. Not so much in the context of a single baseline, but more in the context of updating baselines under source control where they will occupy space in the git history.

With pretty printing, the size of the file is increased by about ~25% (ballpark, I don't have exact figures offhand).

Originally we were going to recommend compressing the baseline in the repo, and uncompressing it before use, or even having SwiftLint compress and uncompress the file itself.

Having done some experiments with git history sizes - see #5553 (comment), it actually looks like its better to store it uncompressed, as that way git's internal packing and compression will actually produce good compression ratios without any extra work on our part.

In the absence of pretty-printing, you can use the baseline command to see what violations are present in the baseline - for example:

swiftlint baseline --reporter summary MyBaseline.json

for a summary, or something like

swiftlint baseline --reporter xcode MyBaseline.json

for individual warnings. You could even run the latter a run script build phase, to see the Baseline violations reported directly within Xcode.

In Periphery there is no equivalent of the baseline command, so for right now, I'm leaving pretty-printing on, but that may change as we work our way though code review etc.

Hope this helps - please let us know if things are not working the way you expected after this.

Any feedback about bugs, usability, whether the documentation is clear enough etc. are very very welcome.

@mildm8nnered
Copy link
Collaborator

mildm8nnered commented May 20, 2024

Incidentally, I was very interested to see https://detekt.dev/docs/introduction/baseline/ and https://googlesamples.github.io/android-custom-lint-rules/usage/baselines.md.html which I think you mentioned somewhere, and which I was not aware of - I will definitely have a closer look at those.

@aiKrice
Copy link
Author

aiKrice commented May 20, 2024

Hello @mildm8nnered , Thank for being so reactive 🚀 .

1️⃣. Baseline generation:
I've tried this command swiftlint --write-baseline baseline.json -config config.yml file.swift but I "didn't" expect this if I compare to Dekekt and AndroidLint. Because this means that I have to provide all files one by one ? In this case is there a parameter for that, I probably miss it. But I really think this could be improve (on a developer experience perspective), meanwhile I will implement a recursive find and update my Medium Article based on that. I prefer this rather than having a huge baseline.

2️⃣. Baseline writing output. I was trapped ! I didn't understand that the output because it said: linting done! found violations and I didn't expect such an output which could lead me that the baseline failed. In this case, I suggest a verbose mode disabled by default or something else. Also I am wondering why if I add a file in the exclusion in my config.yml, it is taken in account in the baseline generation. It should be excluded (it is like this in Detekt and AndroidLint)

3️⃣ . Ok aligned of course. I did it because I though no choice

4️⃣ . If we could provide a pretty printing option it would be nice (i'm aligned of course on what you said).

To sum it up, here are my opinions and suggestions :
Baseline generation command: We could provide a system to browse all swiftlint file (except if I have one)
Baseline output: disabled output by default / provide a verbose
Option for json prettifier.

Overall. Nice !!!

Gonna modify my Medium article with temp solution of recursive swift browsing.

And available for discussing for Android equivalent. The friendly link medium article for how to generate the baseline for AndroidLint or Detekt in a nutshell is in an another article -> https://medium.com/@SaezChristopher/4-best-tools-to-implement-into-your-githooks-in-an-android-project-dfc93e1852b7?sk=a15ad91c046ab14f17614b7c43a486b0

@mildm8nnered
Copy link
Collaborator

Hello @mildm8nnered , Thank for being so reactive 🚀 .

1️⃣. Baseline generation: I've tried this command swiftlint --write-baseline baseline.json -config config.yml file.swift but I "didn't" expect this if I compare to Dekekt and AndroidLint. Because this means that I have to provide all files one by one ? In this case is there a parameter for that, I probably miss it. But I really think this could be improve (on a developer experience perspective), meanwhile I will implement a recursive find and update my Medium Article based on that. I prefer this rather than having a huge baseline.

So you should not have to provide your files one by one at all. For "medium-ish" side projects, for some value of "medium-ish" - mine is quarter of a million lines of Swift code, or thereabouts, enabling about 20 rules that I would have liked to turn on, but couldn't because of existing violations, the baseline is about 2.5MB.

That is not really that large by modern standards. Even with --enable-all-rules, the baseline there for me is about 7.5MB, which is more substantial, but not really that bad. We commit CocoaPods in my main work project, and the SwiftLint binary is 33MB.

I wasn't aware of a lot the prior art when I started, so I can't comment yet on Detekt and AndroidLint (but I am intending to check them out), but in summary, I would say, just invoke SwiftLint exactly how you would normally, with identical command lines in the writing (--write-baseline) and filtering (--baseline) cases, apart from the baseline arguments.

2️⃣. Baseline writing output. I was trapped ! I didn't understand that the output because it said: linting done! found violations and I didn't expect such an output which could lead me that the baseline failed. In this case, I suggest a verbose mode disabled by default or something else. Also I am wondering why if I add a file in the exclusion in my config.yml, it is taken in account in the baseline generation. It should be excluded (it is like this in Detekt and AndroidLint)

So in SwiftLint's case, the baseline just says "ignore and do not report violations that are in the baseline" - there is no real extra output to say "we filtered these many violations".

We could definitely look at adding something at least as a summary "0 violations detected, 6,107 violations filtered", but we don't do that right now.

Reporting filtered violations in detail - that would be difficult. You might want to know "how many violations were filtered", or "how many violations in the baseline are not actually present in the tree any more". You can do that with the baseline command, and it's compare subcommand, but it takes extra work.

4️⃣ . If we could provide a pretty printing option it would be nice (i'm aligned of course on what you said).

To sum it up, here are my opinions and suggestions : Baseline generation command: We could provide a system to browse all swiftlint file (except if I have one)

Could you clarify that a bit, if my response to 1) above doesn't already cover that for you?

Baseline output: disabled output by default / provide a verbose Option for json prettifier.

So swiftlint baseline --reporter json Baseline.json will pretty-print the JSON for you.

Re-using the reporters for this is actually really nice, because a baseline is just a collection of violations, and we have multiple reporters (and can always add more) for reporting violations in various formats.

Overall. Nice !!!

Thanks!

Gonna modify my Medium article with temp solution of recursive swift browsing.

So maybe we could thrash out the issues here, and that might save you having to make multiple edits (and once again, thank you so much for popularizing this).

And available for discussing for Android equivalent. The friendly link medium article for how to generate the baseline for AndroidLint or Detekt in a nutshell is in an another article -> https://medium.com/@SaezChristopher/4-best-tools-to-implement-into-your-githooks-in-an-android-project-dfc93e1852b7?sk=a15ad91c046ab14f17614b7c43a486b0

I am definitely going to check those out as soon as I have a chance!

@aiKrice
Copy link
Author

aiKrice commented May 21, 2024

For info @mildm8nnered . I have performed:

swiftlint --write-baseline config/swiftlint/baseline.json --config config/swiftlint/swiftlint.yml $(find . -name "*.swift")
It works, for sure.

For point 4, tried also, it works with the command. At least no need to install an additionnal tools.

I will perform a last update of the article. Thank you 👍

@mildm8nnered
Copy link
Collaborator

So you should not need to do the $(find . -name "*.swift") trick - SwiftLint should be looking for the *.swift files itself, based on your configuration, just like a normal lint run.

Does that not work for you?

swiftlint --write-baseline config/swiftlint/baseline.json --config config/swiftlint/swiftlint.yml $(find . -name "*.swift") It works, for sure.

For point 4, tried also, it works with the command. At least no need to install an additionnal tools.

I will perform a last update of the article. Thank you 👍

@aiKrice
Copy link
Author

aiKrice commented May 21, 2024

@mildm8nnered Do you see something wrong / missing in my config file above ?
swiftlint --write-baseline config/swiftlint/baseline.json --config config/swiftlint/swiftlint.yml
Output

Linting Swift files in current working directory
Error: No lintable files found at paths: ''

I tried --path . but it says it's deprecated + it doesnt work as well XD

warning: The --path option is deprecated. Pass the path(s) to lint last to the swiftlint command.
Linting Swift files at paths .
Error: No lintable files found at paths: '.'

And my team and I are running under zsh

@mildm8nnered
Copy link
Collaborator

mildm8nnered commented May 21, 2024

So I think it's not your config file per se, but where you've put it (in config/swiftlint). The (very recently) updated readme says "Configure SwiftLint by adding a .swiftlint.yml file from the directory you'll run SwiftLint from."

SwiftLint implicitly expects the main config file to be at the top of the tree, and then can use various mechanisms to refine the configuration.

In your case, SwiftLint is probably trying to scan config/swiftlint/Shopmium etc.

If you need to have the config file in config/swiftlint, I think you can probably adjust the paths (../../Shopmium perhaps), and that should probably work.

@aiKrice aiKrice closed this as completed May 22, 2024
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

No branches or pull requests

2 participants