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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ktlint to improve linting of kotlin files #2543

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ShivamPokhriyal
Copy link
Contributor

Summary

Saw this PR dimagi/commcare-core#1064 and thought it might be a good idea to add ktlint.

https://github.com/pinterest/ktlint is an amazing kotlin linter library which provides a built-in formatter so that we don't have to fix those issues manually(Although I had to fix some of the existing issues manually 馃槄 see 3rd commit).
PR uses https://github.com/pinterest/ktlint/#-with-gradle to add ktlint to the project.

After this change, the danger-lint pr tool should most probably be able to comment these lint issues on the PR(will have to test). Or maybe the lint task would make the PR checks fail(also need to test this).
Regardless, we now have 2 gradle tasks:

  • ./gradlew ktlint this returns all the lint issues in the kotlin files.
  • ./gradlew ktlintFormat will fix those lint issues

PS: We should consider upgrading kotlin version. Current is 1.3.72

Feature Flag

Product Description

Safety Assurance

  • If the PR is high risk, "High Risk" label is set
  • I have confidence that this PR will not introduce a regression for the reasons below
  • Do we need to enhance manual QA test coverage ? If yes, "QA Note" label is set correctly

Automated test coverage

Safety story

Shivam Pokhriyal added 3 commits January 25, 2022 18:56
Ran ./gradlew ktlintFormat
ktlintFormat cannot fix wildcard imports. It fails with cannot be auto-corrected
@damagatchi
Copy link

Can one of the admins verify this patch?

@ShivamPokhriyal
Copy link
Contributor Author

ShivamPokhriyal commented Jan 25, 2022

If the PR is high risk, "High Risk" label is set
I have confidence that this PR will not introduce a regression for the reasons below
Do we need to enhance manual QA test coverage ? If yes, "QA Note" label is set correctly

FYI: I'm unfortunately not able to add labels to the PR.

@shubham1g5
Copy link
Contributor

@damagatchi ok to test

@ShivamPokhriyal
Copy link
Contributor Author

@damagatchi retest this please

1 similar comment
@shubham1g5
Copy link
Contributor

@damagatchi retest this please

@dimagimon
Copy link
Collaborator

1 Warning
鈿狅笍 This PR does not contain any JIRA issue keys in the PR title or commit messages (e.g. KEY-123)

Generated by 馃毇 Danger

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.

None yet

4 participants