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

Updating the linter #364

Closed
Jolanrensen opened this issue Apr 26, 2023 · 5 comments · Fixed by #754
Closed

Updating the linter #364

Jolanrensen opened this issue Apr 26, 2023 · 5 comments · Fixed by #754
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@Jolanrensen
Copy link
Collaborator

The linter plugin we use (https://github.com/jeremymailen/kotlinter-gradle) can be updated to v3.14.0.

However, two things:

@Jolanrensen Jolanrensen added the enhancement New feature or request label Apr 26, 2023
@Jolanrensen Jolanrensen added this to the Backlog milestone Apr 26, 2023
Copy link
Collaborator

Probably we need a strong DETEKT rules here

@Jolanrensen
Copy link
Collaborator Author

Jolanrensen commented May 28, 2024

Relevant issue: pinterest/ktlint#2675 (for accepting this[{ notation }])

PR: pinterest/ktlint#2677

@Jolanrensen Jolanrensen self-assigned this May 29, 2024
@Jolanrensen Jolanrensen modified the milestones: Backlog, 0.14.0 May 29, 2024
@Jolanrensen
Copy link
Collaborator Author

Jolanrensen commented May 29, 2024

After that PR is merged, the linter can be updated relatively easily.

  • We need to wait till there are not so many PRs anymore
  • The linter will format the entire project (!), most things can be done automatically, but the rest can be done in about a day's work, as seen in this example branch: https://github.com/Kotlin/dataframe/tree/k2-linter-attempt
  • This branch also contains a sample .editorconfig file which disables only the rule "standard:filename" and it configures IntelliJ to not have *-imports at all :)
  • Generated code can be excluded
  • Using the IntelliJ KtLint plugin we can more easily see errors and @Suppress() rules if we need to deviate somewhere.
  • We can commit-hook the formatKotlin task so only clean code gets pushed :)
  • We can even write custom rules if we need them (I don't expect we do) https://pinterest.github.io/ktlint/latest/api/custom-rule-set/
  • We should also reformat generated code, to avoid long newline chains as a result of @ExcludeFromSources

@Jolanrensen Jolanrensen changed the title Linting Updating the linter May 29, 2024
@Jolanrensen
Copy link
Collaborator Author

I've been tweaking some settings in .editorconfig to what I think looks best (and most in-line with DataFrame):
https://github.com/Kotlin/dataframe/blob/k2-linter-attempt/.editorconfig

We just need to figure out how to NOT run on generated-sources by default (as it's way to heavy) but allow the linter to format generated-sources from github actions instead.

@Jolanrensen
Copy link
Collaborator Author

Jolanrensen commented Jun 25, 2024

Some updates:

I've had difficulties using Kotlinter due to its limited customization options. After trying Spotless, which also wasn't fit for the job, I found https://github.com/JLLeitschuh/ktlint-gradle, which is also used by kotlin-jupyer.

This branch contains a working project with that plugin: https://github.com/Kotlin/dataframe/tree/k2-ktlint-jlleitschuh-attempt.

In principle, it works the same as Kotlinter; it takes the .editorconfig file as source of truth, but, if we need it, we can programmatically override certain rules. It's ktlint-version independent. It can auto-check and -format when needed and it creates tasks for each individual source set. Since the generated-sources are no sourceSet, it will not run on them by default, which is great :) We can still create a custom ktlint format task for the generated sources though, as I did in :core:ktlintFormatGeneratedSources in the branch. This can run automatically after processKDocsMain and format the generated-sources folder :) (with outputReadOnly = false of couse).

If people want, they can run addKtlintFormatGitPreCommitHook, which will do exactly that, however, let's keep this optional for now.

Finally, the plugin has baseline support https://github.com/JLLeitschuh/ktlint-gradle?tab=readme-ov-file#baseline-support. This means, in theory, we can make the linter only format files that are touched. While this would lower the impact of enabling the new linter, I feel an opportunity to clean up the library like this will not come around soun again.

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

Successfully merging a pull request may close this issue.

2 participants