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

Add support for ktlint fix with -F #224

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

Conversation

smocherla-brex
Copy link
Contributor

@smocherla-brex smocherla-brex commented May 4, 2024

This is a follow-up to ktlint support with an action to patch/autocorrect violations where supported in rules using https://pinterest.github.io/ktlint/1.0.1/install/cli/#format-autocorrect and leveraging existing semantics with rules_lint


Changes are visible to end-users: yes

  • Searched for relevant documentation and updated as needed: yes

Test plan

  • Covered by existing test cases
  • Also verified with ./lint.sh --fix --dry-run that hello.ktwould be updated
From bazel-out/k8-fastbuild/bin/src/ktlint.hello_kt.aspect_rules_lint.patch:
--- a/src/hello.kt
+++ b/src/hello.kt
@@ -1,6 +1,6 @@
-import javax.security.*
 import java.util.*
+import javax.security.*
 
 fun main() {
-  println("Hello, world!")
+    println("Hello, world!")
 }

@smocherla-brex smocherla-brex changed the title Add support for ktlint fix with -F Add support for ktlint fix with -F May 4, 2024
@smocherla-brex smocherla-brex marked this pull request as ready for review May 4, 2024 22:04
Copy link
Member

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

This is confusing, they've shipped a format feature in the lint tool. We already have ktfmt in this repo. Is a user meant to choose one or the other?

@smocherla-brex
Copy link
Contributor Author

smocherla-brex commented May 4, 2024

ktlint acts both as a linter and a formatter (formatting is subset of the standard lint rules). This --format flag is slightly confusing as it goes beyond pure pretty-printing/formatting. For example, custom rules can use this https://github.com/pinterest/ktlint/blob/9b38b37d95fcb0ad90d07d64d581e6059ea8496e/ktlint-api-consumer/src/main/kotlin/com/example/ktlint/api/consumer/rules/NoVarRule.kt#L15 (as an e.g) with -F mapped to autoCorrect in their implementation to suggest custom fixes for the lint rules that'll show up in the diff, so these fixes can also be semantic suggestions/enforcements. That said, I haven't used ktfmt before so if you feel this is redundant, I can close the PR.

@alexeagle
Copy link
Member

I don't want to close the PR - we DO want to have auto-fixes for linters that understand them.
It's a pity that the Kotlin ecosystem got into this confused state - Javascript (eslint) and Python (flake8/ruff) were careful to have linting not fight with formatting. It seems like ktlint authors have freely conflated the two.
We could use ktlint for both purposes, but I don't see a flag that allows you to only format and not lint.

I think we should raise a discussion on the ktlint repo to see if there's any recommended solution (and not make it Bazel specific - we just want to follow what other users do)

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.

2 participants