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

Run ktlint on any push #3760

Conversation

adrianclay
Copy link
Contributor

At the moment it is easy to make some changes and only get feedback that there are linting issues after that change reaching master.

By running ktlint on any push contributors will get feedback sooner. This should hopefully result in fewer linting issues reaching master.

At the moment it is easy to make some changes and only get feedback
that there are linting issues after that change reaching master.

By running ktlint on any push contributors will get feedback sooner.
This should hopefully result in fewer linting issues reaching master.
@westnordost
Copy link
Member

westnordost commented Feb 13, 2022

I'd really not like ktlint to build a regime here. I.e. consistent code formatting is nice but if it becomes a (perceived) barrier in contributing, then it must go. Because really, whether it's when( or when ( is seriously not going to be disturb anyones flow of reading and understanding the code.

Thus, that ktlint does not nitpick so often / so early is deliberate.

Incidentally I activated the ktlint plugin for Android Studio as it is now recommended in a readme somewhere but my experience so far has been that ktlint is a bit annoying. So I am thinking about deactivating it again.

  • For example while one writes code, Android Studio will automatically indent the next line when one presses enter. ktlint will immediately mark this as an "error" and this is really distracting. Sometimes I go like "huh, what is wrong here now? Some wrong variable name? Did Android Studio break again and do I need to press the 'sync from gradle' button again? Ah no, it's just ktlint 🙄"
  • The other issue I got with ktlint is that it marks everything as "error". So if anything ktlint seems to inherently train me to ignore errors while in the process of writing the code. And this is not a good thing

@rugk
Copy link
Contributor

rugk commented Feb 15, 2022

Hehe and this tool calls itself "anti-bikeshedding" well… 😅

On the other hand, it also has a built-in formatter (accoridng to that doc, I don't know the tool), so could not we instead of showing all errors on push, just automatically commit and fix and push them? GitHub actions can commit and push without problems....
Of course, you may need to do git pull again if you want to continue working here, but if you already committed and pushed your changes, you hopefully have finished some part of your work already.

@FloEdelmann
Copy link
Member

By running ktlint on any push contributors will get feedback sooner.

No, push would just work for all branches of this repository; so it would only show in PRs from project members. It would work with the pull_request event though: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows

I initially had it like that (207b5e0 (#3649)), and then even a bit fancier after @matkoniecz's feedback (#3649 (comment)), but @westnordost didn't like it (#3649 (comment)).

@FloEdelmann
Copy link
Member

For example while one writes code, Android Studio will automatically indent the next line when one presses enter. ktlint will immediately mark this as an "error" and this is really distracting.

I agree with that. "Lint on save" instead of "Lint while typing" would help here. I suggested that in a similar issue on the plugin's bug tracker: nbadal/ktlint-intellij-plugin#125 (comment)

The other issue I got with ktlint is that it marks everything as "error".

You can configure that:

grafik

@FloEdelmann
Copy link
Member

it also has a built-in formatter […], so could […] we […] automatically commit and fix and push them?

Yeah that could work. @westnordost would that be okay for you?

@westnordost
Copy link
Member

Thanks for the hint to the configurations! Is there any possibility to add this as default per .editorconfig or similar?

Yeah that could work. @westnordost would that be okay for you?

No. A human must have the final say over how things are formatted, ktlint may only advise.

@westnordost
Copy link
Member

Anyway, I'll close this PR.

@FloEdelmann
Copy link
Member

Is there any possibility to add this as default per .editorconfig or similar?

Yes, I've added the config file to Git in 970eff3.

@westnordost
Copy link
Member

Perfect, thanks!

@FloEdelmann
Copy link
Member

@adrianclay (or anyone reading this): By the way, installing the Ktlint (unofficial) Android Studio plugin like described in the contributing guide will probably solve the issue for your future contributions, as you will notice lint issues right away.

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.

4 participants