Remove ktlint & update detekt to 1.19.0

[ovitrif]

Fixes #14012

Why? Ktlint is no friend of the @Composable annotation on parameters (see paqN3M-v8-p2), and we plan to start adopting Jetpack Compose for building UIs.

📃 Changes:

• Remove ktlint task from CI
• Remove ktlint config & dependency
• Update coding-style doc to suggest using detekt instead of ktlint
• Replace ktlint comments with detekt suppressions
• Fix EmptyFunctionBlock detekt errors by using expression body ( = UNIT)
• Suppress MultiLineIfElse in SubfilterPageViewModelTest
• Suppress MayBeConst detekt errors
• Suppress MaxLineLength detekt error in PostUtilsUnitTest
• Fix and suppress detekt errors in MySiteViewModelTest & restore test lost during a merge
• Bump detekt from 1.15.0 to 1.19.0
• Migrate detekt reports configuration to the recommended task-level approach
• Update detekt baseline to suppress 118 existing errors
• Apply suggestions 2, 6 & 7 from the list of 7 in comment for Detekt Integration - Phase.2 #14011

🤔 Questions:

1. @ParaskP7 Is this point still relevant considering we're adding the ktlint ruleset with the detektPlugins declaration?

   When the decommission process for KtLint starts, the responsible person should traverse all the existing KtLint rules and make sure that those are being transferred to Detekt. Step no.1 about would help with that but still there might be cases where some rules might still required to be ported from KtLint to
   Detekt.

   My understanding from two pages in the detekt docs ([formatting rules] (https://detekt.dev/docs/rules/formatting/) & adding more rule-sets) is that the rules are already included when the detektPlugins dependency is added 🤷‍♂️.

2. Why the changes of rules names in detekt.yml?
   Apparently these have changed with the new version, it was easy enough to figure them out as there were build errors pointing to the deprecated rule names. Mostly this concerns the renames into ignoreAnnotated.

3. Is it ok to add yet another 118 suppressions to the baseline file?
   I started with the idea of fixing all the errors, but they proved to be too many (e.g. most of those are one error per file, which makes the job much more time consuming.
   If agreed we can continue with new suppressions in the baseline and consider fixing these rather sooner than later.
   I did some exploration and I tend to agree with most of the errors outlined, ideally we'd fix them 😃 .

🧪 To test:

• Make sure to read the detekt changelog for the versions greater than 1.15.0 up to 1.19.0 and check the migration notes against the changes to verify the migration was done correctly.
• Pull branch issue/14012-remove-ktlint locally
• Run ./gradlew WordPress:detekt and
  expect a successful build.
• Delete config/detekt/baseline.xml file
  • run ./gradlew WordPress:detektBaseline
  • run git status and
    expect no changes in the detekt/baseline.xml file.
• Locate the Detekt-related pipeline job in Buildkite and expect success.

⚠️ Merge Instructions:

1. Make sure @wordpress-mobile/owl-team approved the PR (@ParaskP7).
2. Remove the 🔴 Not Ready for Merge label.
3. Merge 🚀.

Regression Notes

1. Potential unintended areas of impact
   N/a

2. What I did to test those areas of impact (or what existing automated tests I relied on)
   N/a

3. What automated tests I added (or what prevented me from doing so)
   N/a

PR submission checklist:

• I have completed the Regression Notes.
• I have considered adding accessibility improvements for my changes.
• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

[peril-wordpress-mobile]

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

[wpmobilebot]

You can test the Jetpack changes on this Pull Request by downloading an installable build (jetpack-installable-build-pr16819-480d38c.apk), or scanning this QR code:

[wpmobilebot]

You can test the WordPress changes on this Pull Request by downloading an installable build (wordpress-installable-build-pr16819-480d38c.apk), or scanning this QR code:

[ParaskP7]

👋 @ovitrif !

1. @ParaskP7 Is this point still relevant considering we're adding the ktlint ruleset with the detektPlugins declaration?

Good question, since Detekt is already utilized for some time now, I would suggest us progressing with the KtLint removal without worrying about this point. The default KtLint rules are enabled as part of Detekt anyway and this I think is enough for our needs at the moment. If, at some point, we figured that we are missing a rule from KtLint we can always re-enable it after the fact.

[ParaskP7]

👋 @ovitrif !

2. Why the changes of rules names in detekt.yml?
   Apparently these have changed with the new version, it was easy enough to figure them out as there were build errors pointing to the deprecated rule names. Mostly this concerns the renames into ignoreAnnotated.

👍 🥇 💯

3. Is it ok to add yet another 118 suppressions to the baseline file?
   I started with the idea of fixing all the errors, but they proved to be too many (e.g. most of those are one error per file, which makes the job much more time consuming.
   If agreed we can continue with new suppressions in the baseline and consider fixing these rather sooner than later.
   I did some exploration and I tend to agree with most of the errors outlined, ideally we'd fix them 😃 .

Yes, let's then add them to the baseline and maybe create a few target GitHub issues to fix those as part of separate and multiple PRs, just to make sure that eventually the baseline will at least become as small as it was before this change.

PS: The only other thing I can add here is that if I were you I would have split this PR into 2, one where KtLint is removed and the other one where Detekt is update to 1.19.0. I am not sure whether you needed to update Detekt because otherwise KtLint removal couldn't complete, as I haven't finished with the PR review yet, but I thought I should mention that in response to your questions because if we were to only focus on the KtLint removal in this PR we would have worried too much about these, right?

[ParaskP7]

👋 @ovitrif !

buildkite/wordpress-android/ktlint Expected — Waiting for status to be reported

FYI: If you are, don't worry about this too much. As soon as this PR is reviewed and ready to be merge, I can delete this check from here, as it would not longer be necessary, and so that everything goes 🟢 !

[ParaskP7]

👋 @ovitrif !

Thank you so much for creating this PR, both on the KtLint removal and on the Detekt Upgrade. I have reviewed and tested this PR as per the description, everything works as expected. 👍

However, I do have a but for you on the Detekt Upgrade (see section below)... 🤔

---

Detekt Upgrade

1. There are two ways of configuring Detekt and the detekt.yml file that contains the rules. For WPAndroid we are using buildUponDefaultConfig = false, which means that we totally control the detekt.yml file and need to update that accordingly with every Detekt update as well. For WCAndroid, they are using buildUponDefaultConfig = true, which means that they depend on the default detekt.yml file, per version, and then only override that default with their own configuration (see WCAndroid detekt.yml file). Both approaches have pros and cons, and for what is worth maybe us adopting WCAndroid approach will make it easier for us to update Detekt in the future too. Why you ask? I will connect this point with the below.
2. When using the buildUponDefaultConfig = false approach, that means that in order to update Detekt from one version to another, and make sure that you also respect the new rules and updates to the rules, one would need to re-generate the detekt.yml file and carefully apply the new changes to the diff. For example, you might want to follow a process like the below:

• Delete (or rename) the existing detekt.yml file. Without doing that, you won't be able to generate the new detekt.yml as Detekt will note it already exists.
• Run this Gradle command to generate the default detekt.yml file for this version (1.19.0): ./gradlew WordPress:detektGenerateConfig
• Compare the diff, enable/disable those rules/configuration that we already agreed on.
• Enable/disable any new rules/configuration that come with the new version.
• Run the Gradle command to check whether Detekt passes with the new rule/configuration: ./gradlew WordPress:detekt
• Disable any new rules/configuration that doesn't make sense for WPAndroid.
• Fix any new issues that come from those new rules/configuration.

3. To the above point and if you do it as per the mentioned process, you will notice that the updated detekt.yml file looks quite different to what we have now. I can share a file with you if you would like me to (since I did this work already as part of review). But, doing this work with also add new warnings that we might need to resolve and/or suppress (after disabling Indentation which was enabled by default, I am not counting 173 weighted issues). Thus, I am requesting changes to get us talking about this topic a bit more before merging this to trunk.

Let me know about all the above and we can pair on it if you like. Or, you might want to separate KtLint removal to the Detekt Update work to unblock the Compose work. There is no need to block Compose because of Detekt Update, especially since the change from 1.15.0 to 1.19.0 is quit long, wdyt? 🤔

[ParaskP7]

Minor (🔍): Since the subprojects block is now empty, consider removing it altogether.

[ovitrif]

Thanks 🙇

[ParaskP7]

Minor (🔍): Consider removing the above two KtLint related lines.

[ovitrif]

Oh, did this change and reverted 2 times and now I forgot to remove the two comments 🤦 .
Thanks for spotting it! 🙇

[ovitrif]

I am not sure whether you needed to update Detekt because otherwise KtLint removal couldn't complete, as I haven't finished with the PR review yet

Now after doing the work and learning more context about it I think it would be fine splitting it into two 👍

I wasn't sure of it while working on the PR, because I kept this thought in mind: "you have to make sure the ktlint rules are applied from within detekt" and to be honest I learned what makes that be the case along the way, didn't know it from the start.

but I thought I should mention that in response to your questions because if we were to only focus on the KtLint removal in this PR we would have worried too much about these, right?

True on this 😺 !

[ovitrif]

FYI: If you are, don't worry about this too much. As soon as this PR is reviewed and ready to be merge, I can delete this check from here, as it would not longer be necessary, and so that everything goes 🟢 !

Cool, thank you 🙇‍♂️ 🟢 🚀

[ParaskP7]

👋 @ovitrif !

Now after doing the work and learning more context about it I think it would be fine splitting it into two 👍

Great, maybe this is for the best. Then, I would suggest the following for you (if I may): 😊

• Create another PR, which will focus on the KtLint Removal task only. We will then quickly review and merge it. This should be very straightforward and feel free to add me as a reviewer there too. I am sure I'll just ✅ on it!
• Create a Platform Request for the Detekt Upgrade work. As you saw, our current version of Detekt is the 1.15.0 and the latest is about to be the 1.21.0, which means 6 major versions for Detekt. And at least one of them is quite big I remember (the 1.20.0). Also, and as I noted, it might be beneficial to first switch to buildUponDefaultConfig = true so that to:
  • Make Detekt upgrades easier from now on.
  • Align Detekt's configuration with WCAndroid.

Wdyt? 🤔

I wasn't sure of it while working on the PR, because I kept this thought in mind: "you have to make sure the ktlint rules are applied from within detekt" and to be honest I learned what makes that be the case along the way, didn't know it from the start.

👍 Totally, this makes sense and I want to take half the blame on that! 😅 😊

[ovitrif]

Let me know about all the above and we can pair on it if you like. Or, you might want to separate KtLint removal to the Detekt Update work to unblock the Compose work. There is no need to block Compose because of Detekt Update, especially since the change from 1.15.0 to 1.19.0 is quit long, wdyt? 🤔

Thank you @ParaskP7 for the great insight into how we're using detekt in WPAndroid 🙇

As I mentioned here the reason for updating detekt was to be more sure that we're not losing too many checks that ktlint was doing.

I also played with buildUponDefaultConfig = true and IIRC there aren't more issues reported after updating the baseline.xml.
But I understand that was because we're still adding most rules to detekt.yml, so we're not really building upon default config when most of the rules are overwritten 😃.
I was thinking that seeing no other errors when buildUponDefaultConfig is true means the newer rules that got introduced with the newer detekt version(s) are enabled and the code is aligned with them.

But if the process of regenerating the detekt.yml file and manually re-applying all the rules we had results in more errors reported by ./gradlew detekt then the result of this process is of more value than without doing it.

I do agree it's better to split the work into two PR-s that are concerned only with solving their own problem:

• Remove ktlint
• Upgrade detekt version.

That way following the conversation will also become easier 👍 . I'll do that 🚀

[ovitrif]

• Create another PR, which will focus on the KtLint Removal task only. We will then quickly review and merge it. This should be very straightforward and feel free to add me as a reviewer there too. I am sure I'll just ✅ on it!
• Create a Platform Request for the Detekt Upgrade work. As you saw, our current version of Detekt is the 1.15.0 and the latest is about to be the 1.21.0, which means 6 major versions for Detekt. And at least one of them is quite big I remember (the 1.20.0). Also, and as I noted, it might be beneficial to first switch to buildUponDefaultConfig = true so that to:
  • Make Detekt upgrades easier from now on.
  • Align Detekt's configuration with WCAndroid.

This sounds good @ParaskP7! Thank you 🙇
I will proceed with this plan 👍

[ParaskP7]

Thank YOU @ovitrif , and for anything you might need, I'll be at your disposal ! 🚀 🥇 💯

[ovitrif]

Created a separate PR for handling the Ktlint removal: #16828

[ParaskP7]

Created a separate PR for handling the Ktlint removal: #16828

Great stuff, thank you so much for this @ovitrif , I will take a look at it first thing tomorrow! 🙇 🥇 💯

[ovitrif]

Closing this in favor of:

• the already merged PR to remove Ktlint
  Remove ktlint & update detekt to 1.19.0 #16819
• Apps Platform Request to upgrade Detekt version
  pdnsEh-qb-p2
• Issue to fix the suppressions in baseline.xml that will result with the Detekt upgrade
  Fix detekt suppressed errors in baseline.xml after bump to 1.19.0 #16829

[ParaskP7]

Thank you so much for all this wonderful work and preparation to kick-off work @ovitrif , you rock! ❤️ 🚀 🪨