Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Issue #11866: Upgrade detekt and its configuration to 1.19.0 #11867

Merged
merged 10 commits into from
Mar 15, 2022

Conversation

gabrielluong
Copy link
Member

Fixes #11866. Original PR can be found at #11540. I went through everything line by line comparing our original base config that was from 1.0.0RC6-4 https://github.com/detekt/detekt/blob/RC6-4/detekt-cli/src/main/resources/default-detekt-config.yml to the latest https://github.com/detekt/detekt/blob/v1.19.0/detekt-core/src/main/resources/default-detekt-config.yml trying to align and keep the config as close to the defaults except where we have made known changes to enable a rule. There have been some new rules that were turned on by default over the several versions - I tried to align with them where possible assuming that it did not add to the baseline. Otherwise, I kept the rule off based on the 1.0.0RC6-4 default.

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

@gabrielluong gabrielluong added the 🕵️‍♀️ needs review PRs that need to be reviewed label Mar 15, 2022
@gabrielluong gabrielluong requested a review from jonalmeida March 15, 2022 02:53
@gabrielluong gabrielluong requested a review from a team as a code owner March 15, 2022 02:53
Copy link
Contributor

@jonalmeida jonalmeida left a comment

Choose a reason for hiding this comment

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

Excellent! Keen to see this land sooner in the release cycle and slowly start to enable more checks over time.

@@ -186,24 +282,44 @@ naming:
minimumFunctionNameLength: 3
FunctionNaming:
active: true
functionPattern: '^([a-z$][a-zA-Z$0-9]*)|(`.*`)$'
functionPattern: '([a-z][a-zA-Z0-9]*)|(`.*`)'
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider updating to include support for Compose (i.e. allow capitals for the first character): https://detekt.dev/compose.html#configurations

Suggested change
functionPattern: '([a-z][a-zA-Z0-9]*)|(`.*`)'
functionPattern: '[A-Za-z][a-zA-Z0-9]*)|(`.*`)'

Copy link
Member Author

@gabrielluong gabrielluong Mar 15, 2022

Choose a reason for hiding this comment

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

I think we good here because we have ignoreAnnotated: ['Composable'], which is one of the options provided that I think it's preferable to modifying functionPattern since someone else might just look at this without context (unless we added a comment) and realign it with the default.

Copy link
Contributor

Choose a reason for hiding this comment

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

The docs say that ignoreAnnotated is for the annotation @Composable. Whereas the functionPattern is so that the functions that start with a capital won't be mark as a lint error.

Comment on lines +600 to +601
UseCheckNotNull:
active: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider as nice-to-have, but requires you to re-run baseline.

Suggested change
UseCheckNotNull:
active: false
UseCheckNotNull:
active: true

UseOrEmpty:
active: false
UseRequire:
active: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Oooh, I like, but maybe we can enable this in the future - we throw lots of illegal states.

Suggested change
active: false
active: true

Copy link
Member Author

Choose a reason for hiding this comment

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

Will keep all of these in the follow up. I didn't want to run another lint-baselint if we can just fix up the issues.

active: false
UseOrEmpty:
active: false
Copy link
Contributor

Choose a reason for hiding this comment

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

More nice-to-have.

Suggested change
active: false
active: true

@gabrielluong gabrielluong added the 🛬 needs landing (squash) PRs that are ready to land (squashed) label Mar 15, 2022
@mergify mergify bot merged commit 239684b into mozilla-mobile:main Mar 15, 2022
@gabrielluong gabrielluong deleted the detekt branch March 15, 2022 06:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🛬 needs landing (squash) PRs that are ready to land (squashed) 🕵️‍♀️ needs review PRs that need to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade detekt and its configuration to 1.19.0
3 participants