https://kotlinlang.org logo
Join Slack
Powered by
<https://github.com/kotest/kotest/issues/3918#issu...
# kotest-contributors
s
https://github.com/kotest/kotest/issues/3918#issuecomment-2109824953 as I mentioned here for kotest 6.0 introducing linter would be good idea for major update what do you think? I propose following two changes 1. linter on git hook 2. linter on github actions
e
I like linting, since it makes PRs more focused (less formatting changes polluting the diffs). I know it isn't appreciated by everyone, and I can understand why as well.. Ktlint has added some controversial rules and defaults lately. And the experience when developing mustn't be disturbed too much, but I think Ktlint's intelliJ plugin makes the code conformant and w/o distracting the user. Most first-time contributors will probably miss it though, and have the PR error and then they have to fix it again etc.. Perhaps we could just make GH run the linter and commit the formatted code if it differs, like we do with the API dumps. To summarize my opinion: leaning in favor but not sure.. 🙂
a
Some sort of automatic git-hook formatter could be good. The difficultly is that although the IJ formatter can be run from the command line, it can't be run if an instance is already open... https://www.jetbrains.com/help/idea/command-line-formatter.html
I am intensely against tools like ktlint. It really disrupts development when it starts blocking me from running tests while I'm in the middle of developing something. I just want to quick test to see if what I've written makes sense! I'll make it look pretty later!
đź’Ż 1
o
With other projects, I'm actually using Ktlint in conjunction with the IJ plugin for automatic, consistent, anti-bikeshedding formatting. It needs some configuration, but then formats more consistently than IJ can. And saves me lots of time thinking about how to format. I'll usually let it format on save, and that's it. I do not, however, force Ktlint Gradle runs automatically. What gave me a bad DX is external tools confronting me with a myriad of questionable rules on how code should look like. I'm using IJ's inspections for that and nothing else. So in that context, for auto-formatting via IJ plugin I'm in favor.
e
I don't think Ktlint would block you from running any tests, ever. At least not if you're using the IJ plugin and leave it in Distract-free mode
That's not been my experience at least.. but sometimes I've had issues where I somehow got past the pre-commit hook locally and push something that I expect will be OK and then it fails when running linter on CI.. but the automatic formatter I suggested would avoid that issue
o
I don't think Ktlint would block you from running any tests, ever.
Yeah, as long as we don't make the 
check
or other test task depend on it, we're safe.
a
maybe ktlint has changed, but in other projects that had ktlint it was a real pain. Like, I'd want to try and debug something, so I'd write some bad code, just to check to see what would happen:
Copy code
fun foo(): Int {
  return 1 + 1 // I add this early return, just to see if it breaks

// [ actual function ...]  
}
and ktlint would blow up and say "you can't write code like that!!!" and prevent me from running tests
đź’Ż 1
e
How about introducing it in just one of the projects after split, and we can evaluate how that goes? Try to iron out any kinks with it, and if we ultimately feel it's not worth it we can discard it.
o
Ktlint has changed. It was pretty rough, yet opinionated in the past, but has become a lot better. And with code that does not compile, it would (silently) just not reformat. If you'd like to try, I could just supply an .editorconfig and some setup guide we could check individually whether we like it.
e
I understand I didn't address anything you just said Adam, but it's hard to refute that w/o seeing it happen. I haven't had any experience like that
a
that's good to hear, maybe things have changed, or the project I used didn't have ktlint configured correctly
s
@Emil Kantis when do you think it is right timing to run auto linting on github? if we run liinter on push think it can cause git conflict issue for contributers if worry about error on git-hooks maybe we can run git hook check on pre-commit, pre-push to double check and finally check on github linter
and what do you mean by split?
e
There’s discussions around splitting the kotest repository into 3. One for assertions, one for the engine and one for property-testing
đź‘€ 1
a
Has anyone tried using reviewdog? It looks like there's a ktlint mode https://github.com/ScaCap/action-ktlint I like the idea of a linter providing suggestions in a code review.
o
Wouldn't it mean that we're still spending time to discuss manual formatting? The first time I came into contact with a high-quality formatter was Python's Black (_“Any color you like.”)_:
Black is the uncompromising Python code formatter. By using it, you agree to cede control over minutiae of hand-formatting. In return, Black gives you speed, determinism, and freedom from 
pycodestyle
nagging about formatting. You will save time and mental energy for more important matters.
Blackened code looks the same regardless of the project you're reading. Formatting becomes transparent after a while and you can focus on the content instead.
Black makes code review faster by producing the smallest diffs possible.
I found the reasoning behind it solid and the experience positive throughout. Ktlint can operate the same way. Along with the IntelliJ plugin, we can write code as sloppily as we like and get consistency immediately on save (or reformat, however we want). If we then want minimal diffs in PRs, we can just mandate a Gradle-based ktlint check like we do with apiCheck.
s
@Oliver.O so you suggest activating ktlint on PR request? which means force format code on PR?
o
Not so fast. Ktlint's Gradle plugin provides 
formatKotlin
and 
lintKotlin
. And there's the IntelliJ Ktlint plugin which operates like IJ's integrated formatter, but in a more consistent way. My priorities in this regard would be: 1. Everyone is sufficiently happy with the way auto-formatting works, 2. avoiding manual formatting (and discussing it) saves us time and allows us to focus on what's more valuable, 3. consistent formatting and consistent diffs help with PR reviews. So I'd suggest: • For an accommodation phase (can be quite a while), we'll try a workflow with a properly configured Ktlint and the Ktlint IntelliJ plugin, so that formatting happens in a distraction-free way (e.g. when saving). • If we're happy with how that works, we'll reformat the entire code base once via 
./gradlew formatKotlin
. • If we're still happy, we'll have a CI check 
./gradlew lintKotlin
, which operates along with 
apiCheck
as a requirement (and quality gate) to merge PRs. Does that make sense?
đź‘Ť 1
s
seems great!
for this |Everyone is sufficiently happy with the way auto-formatting works, maybe applying least linting can be somehow can work e.g) indent, trailling comma
a
I'm with @Adam S - linting should not automatically run on each build, only on merging PRs, and when explicitly invoked, so that it does not slow down experimenting with imperfect code.
Having said that, I concur we don't want to look at PRs where 95% of changes are formatting, makes it difficult to review PRs.
s
if we don't want to look at format change in pr there are two ways 1. activate 
ktlintFormat
on git commit and git push 2. activate 
ktlintFormat
on push second option can cause conflict issue so... I think first option is better
e
How would we enforce the linting without having it as part of the builds?
And like I said, ktlint should just fix the formatting for you. It’s not going to forbid you from writing certain code. Just certain formatting
s
I think make individual user run ktlint plugin will harm linter's goal [link](https://stackoverflow.com/questions/29031329/how-to-exclude-certain-tasks-per-default) how about this? by deafult don't have to run ktilnt in build installing ktlint on one's local machine makes ktlint version problem so I suggest we add kitlint on gradle and turn off default build and run lint on git 
commit
and 
push
a
kotest is a hobby for me, not a full time job. as such, I should be able to switch context to work quickly, that is commit/push my branch/do other work. because life happens. of course whatever gets merged into master should be properly formatted, I get it. but I don't like the idea of running ktlint on git commit - it will slow down my switching to other tasks. and it will clearly slow down my experimenting with imperfect code. a woodworking analogy: of course I will clean up my garage after I'm done building that thing. but requiring me to clean up after making every cut and after drilling every hole?
and I need to know in advance if merging PR will fail because linting. this is why we need to be able to run the same step locally and address whatever issues it exposes
e
Have you used ktlint?
We wouldn’t be asking you to clean up after each step. We would be asking that you install a workbench modification that automatically cleans up for you after each step without you needing to do jack. And it wouldn’t forbid you from making any particular cuts or holes.
a
I guess I was barking up the wrong tree then, so sorry about that.
s
ktlint will automatically format for you when you 
commit
or 
push
you may don't like way which formatting works so I suggest as little formatting as possible
o
Sorry, this is factually incorrect.
Open in Slack
PreviousNext
Powered by