Bug 1805070 - Update detekt to v1.22.0 in A-C #292

gabrielluong · 2022-12-11T18:55:53Z

I compared various base configs (1.22.0, 1.19.0 and our original 1.0.0RC6) to note any changes. Most of the changes will have come from the latest default base config which has turned on a lot more rules.

I also annotated rules and configurations where our team has manually checked the default settings and added a link to the original issue (if it exists) or the PR.

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.

GitHub Automation

https://bugzilla.mozilla.org/show_bug.cgi?id=1805070

android-components/build.gradle

android-components/config/detekt.yml

gabrielluong · 2022-12-19T19:05:55Z

android-components/config/detekt.yml

@@ -32,7 +39,15 @@ console-reports:
  #  - 'NotificationReport'
  #  - 'FindingsReport'
  #  - 'FileBasedFindingsReport'
-    - 'LiteFindingsReport'
+  #  - 'LiteFindingsReport'


Uncommented this since there's no reason to really exclude this or any particular reports.

gabrielluong · 2022-12-19T19:08:44Z

android-components/config/detekt.yml

-  LibraryCodeMustSpecifyReturnType:
-    active: true
-    excludes: ['**']
-  LibraryEntitiesShouldNotBePublic:


The library ruleset is now introduced via a new plugin, which I didn't import. See https://detekt.dev/changelog#migration

We can followup on this if you believe we want these library ruleset.

jonalmeida

Thanks, this looks good. The file is quite large to check every change, but maybe we can fixup the config as we go about it? I left some comments on the focus-android detekt that apply for the A-C one too.

android-components/config/detekt.yml

jonalmeida · 2022-12-21T22:06:33Z

focus-android/quality/detekt.yml

  UseCheckNotNull:
    active: true
  UseCheckOrError:
-    active: false
+    active: true


Why make this true? I see that the detekt team are enabling them default true as a way to promote, but I don't see the value it brings. Do you have a stronger opinion to wanting this updated?

See #292 (comment)

jonalmeida · 2022-12-21T22:07:52Z

focus-android/quality/detekt.yml

  UseAnyOrNoneInsteadOfFind:
-    active: false
+    active: true
  UseArrayLiteralsInAnnotations:


Same for this one, the docs say it's more readable but I don't see any reason to make us change our default.

This is enabled by default in detekt since v1.21.0 https://detekt.dev/docs/rules/style/#useanyornoneinsteadoffind. I wouldn't say we really have our own default except where we have a comment that stated that we enabled or changed a property and the PR/issue that was responsible for that decision. Our default is more based on what was the default of the previous base configs.

I would lean towards following the base configuration because keeping this inactive means we have an intentional intervention and now requires another comment about why or when we made this decision. Looking at the code sample, I don't see any reason why we wouldn't want to accept more readable code.

I am also thinking about this from an upgrade point of view where we are currently diff our current config and the new base config to sync any new rules that needs to be added to the file. After this upgrade, I am looking to remove most of the default configurations in favour of using buildUponDefaultConfig.

I am also looking to turn on more rules after this. Especially rules that will make our code more readable and simplifies things for us. Based on our review guidelines, I want to front load and really empower our linters to do most of the heavy lifting by being more opinionated because most of my review codes are based around simplifying and making code more readable.

This is enabled by default in detekt since v1.21.0 https://detekt.dev/docs/rules/style/#useanyornoneinsteadoffind.

I was referring to the line I commented on, UseArrayLiteralsInAnnotations. Using any or none sounds fine to me.

Our default is more based on what was the default of the previous base configs.

This is not true. We chose our defaults intentionally based on our needs.

Looking at the code sample, I don't see any reason why we wouldn't want to accept more readable code.

I am all for this, but using defaults aren't adding that value, it is just code churn. UndocumentedPublicClass, UnreachableCatchBlock and CognitiveComplexMethod are valuable to the code base. The detekt defaults are made by the developers that want to switch some changes to true because they invested in building those but that does not mean those make sense for us.

making code more readable.

Great! Some of these checks are not readable to me though (and maybe others). 😅

Ongoing discussion.

csadilek

@jonalmeida @gabrielluong I've looked through the rules now, and thought about your discussion:

I like the adjustments of existing rules that have already been active, but recommend that we do not introduce new rules, as part of this PR. This is so the PR can be unblocked (it's been around for a while), and because I think some of these rules require more discussion e.g., InjectDispatcher, which we already violate in various places, and just ignoring those cases isn't good either.

I recommend landing this without introducing any new rules, but with the refinements, and putting up separate PRs for specific new rules. On those PRs, we can then also include the required refactorings to actually follow the new rules. This will allow us to properly judge if we want to introduce these new rules or not.

csadilek · 2023-02-16T23:53:16Z

focus-android/quality/detekt.yml

  OutdatedDocumentation:
    active: false
    matchTypeParameters: true
    matchDeclarationsOrder: true
+    allowParamOnConstructorProperties: false


👍 This one is good, because I remember complaining about this in reviews a lot :)

csadilek · 2023-02-16T23:55:33Z

focus-android/quality/detekt.yml

+    thresholdInFiles: 26 # (Default: 11) Increased in https://github.com/mozilla-mobile/android-components/pull/9927
+    thresholdInClasses: 26 # (Default: 11) Increased in https://github.com/mozilla-mobile/android-components/pull/9927
+    thresholdInInterfaces: 26 # (Default: 11) Increased in https://github.com/mozilla-mobile/android-components/pull/9927
+    thresholdInObjects: 26 # (Default: 11) Increased in https://github.com/mozilla-mobile/android-components/pull/9927
    thresholdInEnums: 11
    ignoreDeprecated: false
    ignorePrivate: false


Up to here, the refinement of rules that have already been active look good to me!

mergify · 2023-03-02T17:18:58Z

This pull request has conflicts when rebasing. Could you fix it @gabrielluong? 🙏

gabrielluong · 2023-05-05T14:34:46Z

This is just waiting for me to go through the review feedback and making the changes.

gabrielluong · 2023-05-12T15:20:53Z

@jonalmeida This is ready for a review. There's no new rules that are turned on in this iteration. I added a comment next to each rule that differ from the default detekt configuration to indicate that these are turned off intentionally. That way when we need to bump the detekt version and update our config again we will hopefully not just take the defaults as is.

The configs in Fenix and Focus are directly copied from the AC config.

mergify · 2023-05-15T23:29:46Z

This pull request has conflicts when rebasing. Could you fix it @gabrielluong? 🙏

… the team Add a note to each rule and configuration to note their original default value and the issue or PR that modified the rule.

jonalmeida

LGTM! I only reviewed the AC version on the understanding that the Fenix and Focus ones are identical.

android-components/config/detekt.yml

gabrielluong added the 🕵️‍♀️ needs review PRs that need to be reviewed label Dec 11, 2022

gabrielluong requested a review from jonalmeida December 11, 2022 18:55

gabrielluong assigned jonalmeida Dec 11, 2022

gabrielluong commented Dec 11, 2022

View reviewed changes

android-components/build.gradle Show resolved Hide resolved

gabrielluong commented Dec 11, 2022

View reviewed changes

android-components/config/detekt.yml Show resolved Hide resolved

gabrielluong marked this pull request as draft December 11, 2022 19:01

gabrielluong added work in progress Not ready to land yet. Work in progress (WIP). 🕵️‍♀️ needs review PRs that need to be reviewed and removed 🕵️‍♀️ needs review PRs that need to be reviewed work in progress Not ready to land yet. Work in progress (WIP). labels Dec 11, 2022

gabrielluong marked this pull request as ready for review December 11, 2022 21:57

gabrielluong force-pushed the 1805070-update-detekt-in-ac branch 2 times, most recently from c4db5af to cb00b2a Compare December 15, 2022 22:16

gabrielluong mentioned this pull request Dec 16, 2022

Bug 1806170 - Align detekt config between Focus and Android Components #372

Merged

4 tasks

gabrielluong force-pushed the 1805070-update-detekt-in-ac branch 6 times, most recently from 84addcc to aeab1bb Compare December 18, 2022 17:01

gabrielluong commented Dec 19, 2022

View reviewed changes

jonalmeida previously approved these changes Dec 21, 2022

View reviewed changes

csadilek reviewed Feb 17, 2023

View reviewed changes

gabrielluong force-pushed the 1805070-update-detekt-in-ac branch from aeab1bb to af35639 Compare February 23, 2023 21:42

rvandermeulen mentioned this pull request Mar 10, 2023

Update detekt to version 1.21.0 mozilla-mobile/reference-browser#2056

Closed

gabrielluong removed the 🕵️‍♀️ needs review PRs that need to be reviewed label May 5, 2023

gabrielluong force-pushed the 1805070-update-detekt-in-ac branch from af35639 to d1ffb5e Compare May 12, 2023 06:15

gabrielluong requested a review from jonalmeida May 12, 2023 13:50

gabrielluong added the 🕵️‍♀️ needs review PRs that need to be reviewed label May 12, 2023

gabrielluong added 4 commits May 17, 2023 11:42

Bug 1805070 - Part 1: Update detekt to v1.22.0 in A-C

1f0002c

Bug 1805070 - Part 2: Annotate rules that were manually configured by…

0813c4f

… the team Add a note to each rule and configuration to note their original default value and the issue or PR that modified the rule.

Bug 1805070 - Part 3: Sync Focus detekt configuration with AC

8a44516

Bug 1805070 - Part 4: Sync Fenix detekt configuration with AC

eab6005

gabrielluong force-pushed the 1805070-update-detekt-in-ac branch from d1ffb5e to eab6005 Compare May 17, 2023 15:54

jonalmeida approved these changes May 18, 2023

View reviewed changes

android-components/config/detekt.yml Show resolved Hide resolved

gabrielluong added 🛬 needs landing PRs that are ready to land and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels May 18, 2023

github-actions bot added the 🕵️‍♀️ needs review PRs that need to be reviewed label May 18, 2023

gabrielluong force-pushed the 1805070-update-detekt-in-ac branch from 13a0712 to eab6005 Compare May 18, 2023 21:26

Merge branch 'main' into 1805070-update-detekt-in-ac

9ffc8e5

github-actions bot added approved PR that has been approved and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels May 18, 2023

Merge branch 'main' into 1805070-update-detekt-in-ac

ea3fd8f

mergify bot merged commit cfc45b8 into mozilla-mobile:main May 19, 2023
293 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1805070 - Update detekt to v1.22.0 in A-C #292

Bug 1805070 - Update detekt to v1.22.0 in A-C #292

gabrielluong commented Dec 11, 2022 •

edited by github-actions bot

gabrielluong Dec 19, 2022

gabrielluong Dec 19, 2022

jonalmeida left a comment

jonalmeida Dec 21, 2022

gabrielluong Dec 21, 2022

jonalmeida Dec 21, 2022

gabrielluong Dec 21, 2022 •

edited

gabrielluong Dec 21, 2022 •

edited

jonalmeida Dec 22, 2022

csadilek left a comment •

edited

csadilek Feb 16, 2023

csadilek Feb 16, 2023

mergify bot commented Mar 2, 2023

gabrielluong commented May 5, 2023

gabrielluong commented May 12, 2023 •

edited

mergify bot commented May 15, 2023

jonalmeida left a comment

Bug 1805070 - Update detekt to v1.22.0 in A-C #292

Bug 1805070 - Update detekt to v1.22.0 in A-C #292

Conversation

gabrielluong commented Dec 11, 2022 • edited by github-actions bot

Pull Request checklist

After merge

GitHub Automation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonalmeida left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielluong Dec 21, 2022 • edited

Choose a reason for hiding this comment

gabrielluong Dec 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csadilek left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Mar 2, 2023

gabrielluong commented May 5, 2023

gabrielluong commented May 12, 2023 • edited

mergify bot commented May 15, 2023

jonalmeida left a comment

Choose a reason for hiding this comment

gabrielluong commented Dec 11, 2022 •

edited by github-actions bot

gabrielluong Dec 21, 2022 •

edited

gabrielluong Dec 21, 2022 •

edited

csadilek left a comment •

edited

gabrielluong commented May 12, 2023 •

edited