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

Lint Kotlin examples #3923

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

Lint Kotlin examples #3923

wants to merge 16 commits into from

Conversation

myyk
Copy link
Contributor

@myyk myyk commented Nov 8, 2024

Last change I think needed for #3829

Overview:

  • Changes KtfmtModule to set the error code when there's changes like is true of the other formatters.
  • Some simplifications were made to the example/package.mill based on similarities with Java/Kotlin.
  • Files were formatted
  • CI was updated
  • Not sure if KtfmtModule tests need updating as they aren't running cleanly from main on my machine.

test("testSimple") {
generateHtml("hello") shouldBe "<h1>hello</h1>"
}
class FooTest :
Copy link
Contributor

@0xnm 0xnm Nov 8, 2024

Choose a reason for hiding this comment

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

this doesn't look like a reasonable change, but I don't know ktfmt default rules. Overall, in the Kotlin ecosystem ktlint is much more popular than ktfmt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just the default ktfmt as there's no configuration. I'm guessing it's better to any formatter in place and we can let which the formatter battle to happen in parallel since it should then be a fairly easy switch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then why not use ktlint (as it is proposed in #3829) provided by KtlintModule? It will require some changes to the run in the module-independent manner, but it will be easy to do by looking on KtfmtModule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, it is ktlint in the issue. I misread that. Forgive me, I am actually dyslexic.

Copy link
Member

Choose a reason for hiding this comment

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

No worries, let's try ktlint and see how it looks. Then we can pick the one that looks better.

I noticed KtlintModule does not have the companion object extends External Module that the other autoformatters do, making it more annoying to use ad-hoc. We should probably add such an companion external module. KtfmtModule, ScalafmtModule, and PalantirFormatModule can be used as a reference as to how it is done

@myyk myyk mentioned this pull request Nov 10, 2024
1 task
@myyk
Copy link
Contributor Author

myyk commented Nov 10, 2024

I think I need to adjust the linter tests bc I've changed the implementation. Or change the default to not see the status code.

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.

3 participants