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

Install ktlint plugin #3957

Merged
merged 10 commits into from
Oct 1, 2021
Merged

Install ktlint plugin #3957

merged 10 commits into from
Oct 1, 2021

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented Sep 2, 2021

Draft for now.

Iteration call to ./gradlew ktlintCheck does not give the same result :/

[EDIT]

It's OK now, ktlint plugin is working as expected.

The following rule are now enabled (were disabled before):

  • colon-spacing
  • chain-wrapping
  • import-ordering

Fixes #2820

@erikhuizinga
Copy link
Contributor

Try running ./gradlew ktlintCheck --continue. If that leads to identical results, it might be an indication of ktlint running checks randomly. That might explain the non-determinism. I think that the ktlint Gradle tasks fail on the first found problem, while there might be more to find after the first.

@bmarty bmarty marked this pull request as ready for review September 30, 2021 15:59
@bmarty
Copy link
Member Author

bmarty commented Sep 30, 2021

There is still a problem, Android Studio does not always order imports correctly :/

@github-actions
Copy link

github-actions bot commented Sep 30, 2021

Unit Test Results

  42 files  ±0    42 suites  ±0   54s ⏱️ ±0s
  83 tests ±0    83 ✔️ ±0  0 💤 ±0  0 ±0 
220 runs  ±0  220 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 587c634. ± Comparison against base commit 587c634.

♻️ This comment has been updated with latest results.

@erikhuizinga
Copy link
Contributor

I think that the import ordering issues have been discussed extensively on the ktlint repo. Note that various settings can affect the import ordering, e.g. Android Studio settings for the Kotlin language and for the project. I can't check right now, but there might be a ktlint setting that tells ktlint that the project uses IntelliJ / AS. Ktlint then knows that the import ordering should use different rules.

@erikhuizinga
Copy link
Contributor

See also https://github.com/pinterest/ktlint#custom-ktlint-specific-editorconfig-properties.

@bmarty
Copy link
Member Author

bmarty commented Oct 1, 2021

Thanks @erikhuizinga . I've force push the branch with latest improvement.
First commit 03bdcee is the important change, next commits are done by enabling disabled rules one by one and run ./gradlew ktlintFormat

@bmarty
Copy link
Member Author

bmarty commented Oct 1, 2021

So the ktlint plugin is now working as expected.

The following rule are now enabled (were disabled before):

  • colon-spacing
  • chain-wrapping
  • import-ordering

Copy link
Member

@BillCarsonFr BillCarsonFr left a comment

Choose a reason for hiding this comment

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

I'd prefer to keep && and || formating as it is currently

Copy link
Contributor

@ouchadam ouchadam left a comment

Choose a reason for hiding this comment

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

thanks for this 💯

some of the line breaks for closing fun parameters/constructors etc are a little strange with

class Foo(val prop: Long) : 
  RealmObject() {
}

but they should be easy enough to fix later on with a find replace/regex

would have expected to see either...

class Foo(val prop: Long) : RealmObject() {
  ...
}

or

class Foo (
    val prop: Long
) : RealmObject() {
  ...
}

@bmarty
Copy link
Member Author

bmarty commented Oct 1, 2021

I'd prefer to keep && and || formating as it is currently

Yes, me too, but see pinterest/ktlint#163 (comment)

@bmarty
Copy link
Member Author

bmarty commented Oct 1, 2021

@ouchadam yes, I agree with you,some constructors are not nicely formatted. Your latter proposal is good to me. As you said we can manually change that later, I wanted to keep manual commits as minimal as possible in this PR, since it's not possible to review it...

@bmarty bmarty enabled auto-merge October 1, 2021 15:21
@bmarty
Copy link
Member Author

bmarty commented Oct 1, 2021

Fixes #2820

@bmarty bmarty disabled auto-merge October 1, 2021 15:43
@bmarty bmarty merged commit 587c634 into develop Oct 1, 2021
@bmarty bmarty deleted the feature/bma/ktlint_cleanup branch October 1, 2021 16:03
@bmarty bmarty added this to the Sprint - Element 1.3.2 milestone Oct 1, 2021
@erikhuizinga
Copy link
Contributor

Just under 600 files changed... 😅

Agreed: if ktlint accepts the current formatting, then this PR is acceptable. Apply the boy scout rule later when you work on lines of code with strange formatting.

For class definitions with constructor arguments I prefer oneliners, but if too long I prefer to put arguments in one column, possibly even with a trailing comma after the last argument. The same goes for long lists of super types: first one comes after ): and the rest on their own lines.

Is that how the current PR configure ktlint, so what I just said is acceptable formatting?

@bmarty
Copy link
Member Author

bmarty commented Oct 2, 2021

another advantage is that dependabot is now able to upgrade the tool: #4138 :)

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

Successfully merging this pull request may close these issues.

Use a ktlint Gradle plugin to automate ktlint use instead of requiring manual downloading of the ktlint binary
4 participants