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

Remove ktlint #16828

Merged
merged 4 commits into from
Jun 28, 2022
Merged

Remove ktlint #16828

merged 4 commits into from
Jun 28, 2022

Conversation

ovitrif
Copy link
Contributor

@ovitrif ovitrif commented Jun 27, 2022

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.
  • We also have a point on the Android Roadmap (see paqN3M-C6-p2) to remove KtLint in favor of Detekt.

📃 Changes:

  • Remove ktlint task from CI (buildkite pipeline)
  • Remove ktlint configuration & dependency
  • Update coding-style doc to suggest using detekt instead of ktlint
  • Remove ktlint comments for rules suppression

🧪 To test:

  • Locate the Detekt-related pipeline job in Buildkite and expect success.

⚠️ Merge Instructions:

  • Make sure @wordpress-mobile/owl-team approved the PR (@ParaskP7).
  • Remove the 🔴 Not Ready for Merge status label.
  • @ParaskP7 will have to delete the buildkite/wordpress-android/ktlint branch protection rule on trunk.
  • CI should be 🟢 now.
  • 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
Copy link

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

@ovitrif ovitrif added this to the 20.3 milestone Jun 27, 2022
@wpmobilebot
Copy link
Contributor

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

@wpmobilebot
Copy link
Contributor

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

@ParaskP7 ParaskP7 self-assigned this Jun 28, 2022
Copy link
Contributor

@ParaskP7 ParaskP7 left a comment

Choose a reason for hiding this comment

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

👋 @ovitrif !

Thank you so much for re-creating this PR. As per the previous PR, I have reviewed and tested this PR as per the description, everything works as expected. 👍

@ParaskP7 ParaskP7 merged commit 48f593e into trunk Jun 28, 2022
@ParaskP7 ParaskP7 deleted the issue/14012-remove-ktlint-2 branch June 28, 2022 08:04
@ovitrif
Copy link
Contributor Author

ovitrif commented Jun 28, 2022

👋 @ovitrif !

Thank you so much for re-creating this PR. As per the previous PR, I have reviewed and tested this PR as per the description, everything works as expected. 👍

Thank you @ParaskP7 for reviewing and helping with getting this right 🙇 ❤️ 🚀

@ParaskP7
Copy link
Contributor

👋 @ovitrif !

Once more thank you so much for working on removing KtLint from WPAndroid, we are finally free! 🎉

To follow up on the merge could you also:

  1. Create an Apps Platform Request for the Detekt Upgrade work. PS: The details you added here is a good summary.
  2. Close Remove ktlint & update detekt to 1.19.0 #16819 in favor of this PR and the Apps Platform Request.

🥇 🙇 💯

@ovitrif
Copy link
Contributor Author

ovitrif commented Jun 28, 2022

To follow up on the merge could you also:

✅ Done! see #16819 (comment) 💯 🎉 🚀

@ParaskP7 Many thanks for making this possible ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detekt Integration - Phase.3
3 participants