Add ktlint Kotlin linter

[FloEdelmann]

Before reading on: Yes, there are an awful lot of changes in this PR. But I think it's worth it.

About ktlint

ktlint is an easy-to-use linter for Kotlin. Most of the reported issues can also be fixed automatically. It tries to implement the official Kotlin code style without having to configure anything.

It is configurable though: Rules we don't like can be disabled through .editorconfig.

And it is extensible: We could write a custom ruleset someday that e.g. checks the property order in quest type classes (see #3526 and 2fcfd10) – with a real AST, not with hacky regexes.

How to run ktlint

• The ktlint binary can be installed and run manually as a CLI tool. That is how I did it locally.
  • Then just run ktlint in the repo root to check all Kotlin files, or ktlint --format to fix most reported issues automatically.
  • You can run ktlint --apply-to-idea-project to adjust the formatting settings of Android Studio to match what ktlint would produce.
  • You can run ktlint --install-git-pre-commit-hook or ktlint --install-git-pre-push-hook to automatically run ktlint before git commiting / git pushing, so lint issues don't find their way into the repo at all.
• There are Maven and Gradle integrations. I haven't checked those, as I'm not that familiar with those tools.
• On GitHub Actions, we can run ktlint on certain events with the ktlint-check action. I added it to this PR.
  • Please see my demo PR for how that looks: The check fails because there are lint issues; a GitHub comment with an explanation is created automatically in this case. See here for the workflow output. Go to the files tab to see the lint issues inline as annotations directly in the code changes. Also, there are a few unchanged files with check annotations at the bottom – that shouldn't happen if lint issues are regularly fixed/prevented.
  • The workflow is configured to run in every PR and on every push to the master branch. That way, lint issues should be caught quite quickly.

Why we should add it

A more consistent codebase is more maintainable for everyone, because there are less decisions to think about. People that are new to the project learn the code style faster, because they are not confused by existing code that actually violates it. There is less to check in code reviews, because the lint issues are caught by an automatic tool already. So it might mitigate #3573 (comment) a bit.

What the linter complains about

That is what makes this PR so huge: There are many parts in the codebase that violate the ktlint code style. I have let ktlint fix most of the issues automatically, and I have not reviewed the changes thoroughly. I did notice some ugly formatting though, e.g. here. I'll fix that later, if needed.

How to proceed

1. Question to @westnordost and others: Do you like the idea of an automated linter at all?
2. If yes, then we need to decide if we want to disable any rules. I'd say no, but let's hear your opinions. To help with the decision, please go through this PR's commits that start with Lint: and see what code changes the respective rule causes. Please also see this comment for discussions about specific rules.
3. The rules that we keep enabled should be fixed, and the resulting formatting should look good. That means, that I myself will go through the commits once more, and check the output thoroughly. Where needed, I'll add more manual lint changes that are in line with the code style.
4. We need to decide what to do with open PRs. The big list of changes will cause merge conflicts all over the place. Should we take the time once to fix the conflicts in all open PRs? I'd volunteer to do that where I have the privileges to do so.

[FloEdelmann]

Next iteration: The workflow now fails again if there are lint issues, but clicking "Details" immediately opens the list of lint issues. Also, a comment is automatically created that explains what to do now. See again the demo PR.

"Proposed edits" like @matkoniecz suggested seem to be too difficult to implement.

Also, I did now try the Gradle plugin, but it requires manual configuration in Android Studio, so I think it's too complicated. However, Android Studio's code style settings are now checked into git, so just formatting the code inside Android Studio now fixes most lint issues.

[FloEdelmann]

I'll summarize the discussions about specific rules in this comment, and link to the original comments from here. Then they can be hidden/collapsed to keep this thread manageable.

I'll also highlight the (current) concensus: ✔ accepted, ⛔ rejected, ❔ unclear.

✔ 21bb711: remove empty lines before closing curly brace (no-blank-line-before-rbrace)

✔ 5c3da20: Collapse consecutive empty lines (no-consecutive-blank-lines)

✔ b62784e: Add newline to end of files (final-newline)

✔ 3170b09: Standardize spacing around colons (colon-spacing)

• @matkoniecz wrote: Here I probably only need to adjust to the new style

• @FloEdelmann wrote: I actually already tried to conform to exactly these rules until now. As I learned the code style from looking at other code in the project, this must have been predominant already (though inconsistent as well). But I also think the rules makes sense: when is like if, so spacing should be the same. Colons for variable typing have a different meaning than colons before superclasses/interfaces, so spacing should differ.

• @westnordost wrote: This seems inconsistent to me.

• @smichel17 wrote: From https://kotlinlang.org/docs/coding-conventions.html#colon

  Put a space before : in the following cases:

  • when it's used to separate a type and a supertype
  • when delegating to a superclass constructor or a different constructor of the same class
  • after the object keyword

  Don't put a space before : when it separates a declaration and its type.

  I have no opinion on this, just sharing where it came from.

✔ 7433d33: Standardize spacing around commas (comma-spacing)

✔ 3ac056e: Standardize spacing around curly braces (curly-spacing)

✔ 8256af0: Standardize spacing around keywords (if, else, when, …) (keyword-spacing)

• @matkoniecz wrote: Here I probably only need to adjust to the new style
• @FloEdelmann wrote: I actually already tried to conform to exactly these rules until now. As I learned the code style from looking at other code in the project, this must have been predominant already (though inconsistent as well). But I also think the rules makes sense: when is like if, so spacing should be the same. Colons for variable typing have a different meaning than colons before superclasses/interfaces, so spacing should differ.

✔ 64fe90c: Standardize spacing around operators (+, *, !=, …) (op-spacing)

⛔ 5964e14: Standardize spacing around parentheses (paren-spacing)

• @matkoniecz wrote: Nitpicking ahead: is it an improvement to remove alignment?
• @FloEdelmann wrote: Hmmm. Both rules (no-multi-spaces and paren-spacing) catch a lot of tiny inconsistencies, but also remove deliberate formatting. However, such custom spacing is always fiddly: When adding a new line to such places, you often need to add spaces to all other lines to keep everything aligned. Sometimes, with long names/conditions, it is actually less readable. Also, I can never remember whether to put the spaces before or after an -> arrow. See also house name and house number answer at once #3582 (comment). So I think it's best to actually forbid it (that's a clear rule at least). As I said, I will go over all places touched by this PR again and manually improve the formatting. I think we can often find a style that passes these rules and also keeps the readability.

✔ e191c1f: Standardize spacing around comments (comment-spacing)

✔ 57eaaad: Standardize parameter list wrapping (parameter-list-wrapping)

⛔ 8896b71: Standardize logic operator chaining position (chain-wrapping)

• @matkoniecz wrote: Nitpicking ahead: && prefixing is less annoying when you add one more rule - more obvious to add, nicer diffs
• @FloEdelmann wrote: Indeed. I think I'm also voting for disabling this rule.
• @smichel17 wrote: I'm much in favor of && prefixing. I know it's not standard Kotlin style, but it's easier for me to follow and, in my opinion, makes mistakes like fb8fdd4 more obvious (although the linter will not catch them on its own).

---

• @westnordost wrote: Why would that be better than previously?
• @FloEdelmann wrote: It isn't; we already came to that conclusion in the summary. I didn't want to start with reverting specific commits just yet though.

⛔ 9c2f170: Standardize indentation (indent)

• @westnordost wrote: This was deliberate, to keep the number of indentations as low as possible because I think it does improve redability.

• @westnordost wrote: I find the way it was before more readable.

• @westnordost wrote: This is a good example for why I think the style before was better = easier to read.

  Maybe this is a matter of taste, but I got a problem with this deeper level on indentation and all those free floating ) everywhere

• @westnordost wrote: This is another. TBH, I really don't like a linter that would force when to make a newline and when not to. This decision should be up to the human, because in the end, the human also needs to read this. Linters should really limit themselves to small stuff like removing dangling whitespaces, wrong indentation and stuff like this. Not stuff that may be a matter of taste.

⛔ 9f3b543: Collapse multiple consecutive spaces (no-multi-spaces)

• @matkoniecz wrote: Nitpicking ahead: is it an improvement to remove alignment?
• @FloEdelmann wrote: Hmmm. Both rules (no-multi-spaces and paren-spacing) catch a lot of tiny inconsistencies, but also remove deliberate formatting. However, such custom spacing is always fiddly: When adding a new line to such places, you often need to add spaces to all other lines to keep everything aligned. Sometimes, with long names/conditions, it is actually less readable. Also, I can never remember whether to put the spaces before or after an -> arrow. See also house name and house number answer at once #3582 (comment). So I think it's best to actually forbid it (that's a clear rule at least). As I said, I will go over all places touched by this PR again and manually improve the formatting. I think we can often find a style that passes these rules and also keeps the readability.
• @westnordost wrote: See my first comment, I think it is not as well readable as before.

✔ 6b9db3b: Simplify variables in string templates (string-template)

✔ 4f4a79e: Standardize import order (import-ordering)

✔ 511be48: Make wildcard imports explicit (no-wildcard-imports)

• @matkoniecz wrote: is it going to be undone by Android Studio? Sometimes its "autofix missing import" is doing this.

• @FloEdelmann wrote: Actually, I fixed it manually by pressing Ctrl+Alt+o (optimize imports) in Android Studio. There were only a few cases where that didn't work; there I removed one wildcard import at a time and used Android Studio's import suggestions. So it seems that Android Studio won't reintroduce wildcard imports by itself.

• @smichel17 wrote: There's a setting. I believe the default is that it switches to a wildcard if there's more than 3 imports from the same package. I always turn it to "never". Maybe that's already set in the configuration file now checked into the repo.

• @FloEdelmann wrote: Exactly. That's defined in

  StreetComplete/.idea/codeStyles/Project.xml

  Lines 9 to 10 in ffdac58

  	<option name="NAME_COUNT_TO_USE_STAR_IMPORT" value="2147483647" />
  	<option name="NAME_COUNT_TO_USE_STAR_IMPORT_FOR_MEMBERS" value="2147483647" />

• @westnordost wrote: Why are wildcard imports considered to be bad?

• @smichel17 wrote: Wildcard imports make it harder to browse the source code as plain text. Explicit imports means I can search in the file to find where something is imported from.

  Why are explicit imports bad? That's a rhetorical question; we've had this conversation before, so I know the answer. The point of the question is to show that "you should use better tooling" works in both directions:

  A smart IDE can auto-import and collapse the imports, making them effectively equivalent to wildcards, for people using the IDE. People who opt for simple tooling do not have this option, so we should style for them by default.

✔ b622664: Remove unused imports (no-unused-imports)

✔ 96dea18: Rename file to match class name (filename)

[westnordost]

I've finally took the time to look at this.

Question to @westnordost and others: Do you like the idea of an automated linter at all?

1. If yes, then we need to decide if we want to disable any rules. I'd say no, but let's hear your opinions. To help with the decision, please go through this PR's commits that start with Lint: and see what code changes the respective rule causes. Please also see this comment for discussions about specific rules.
2. The rules that we keep enabled should be fixed, and the resulting formatting should look good. That means, that I myself will go through the commits once more, and check the output thoroughly. Where needed, I'll add more manual lint changes that are in line with the code style.
3. We need to decide what to do with open PRs. The big list of changes will cause merge conflicts all over the place. Should we take the time once to fix the conflicts in all open PRs? I'd volunteer to do that where I have the privileges to do so.

So-so. The rules as summarized by you are okay. I'd not like to loose the ❔stuff though, I think it makes it more readable.

What I definitely don't want, and I think Mateusz mentioned it before, is some automatic lint check on GitHub, that prompts users to change their formatting when they create a PR, as seen here.
Instead, if this is possible, this should be done via Android Studio while typing the code. Best solution would be if there was a way to configure this similar to .editorconfig so that everyone who checks out this project and uses Android Studio will get the same code style applied automatically. Is this possible?
If this is not possible, then running this lint every now and then manually (via Android studio, or gradle, or Github Actions) would be fine for me. After a first run of this linter, I don't expect much inconsistencies growing back into the codebase again.

[FloEdelmann]

Thanks for the review @westnordost!

Regarding the rules: The ones you don't like are indent (which hasn't been raised before by @matkoniecz and @smichel17), the ❔ ones (paren-spacing and no-multi-spaces) and the ⛔ one (chain-wrapping). Let's disable them all then. I'll update the summary with your comments.

What I definitely don't want, and I think Mateusz mentioned it before, is some automatic lint check on GitHub, that prompts users to change their formatting when they create a PR, as seen here.

Why not? Do you think it's intimidating for new contributors? Or that it's a waste of time for them?

I'd argue that it actually helps save time for both contributors and reviewers: When commits with lint issues are pushed to a PR, the contributor won't know at first. Then once a reviewer looks over the PR and notices a lint error, they have to make a decision whether to ask the contributor to fix it, or do it themselves now or later, or just ignore it.

Only in the first case will the contributor learn that they should have formatted something in specific way; and they will only learn "why" if the reviewer has put effort into explaining that. An automatic lint check can do that immediately without the contributor having to wait for a review. And we can customize that message to make it less intimidating and more helpful. Also note that a failing check does not prevent merging (although you could require certain checks to pass in the repo settings).

So in the best case, this exact lint error is avoided right from the start the next time. In the worst case, the contributor is so intimidated that they refrain from ever contributing again. I find that quite unlikely.

After a first run of this linter, I don't expect much inconsistencies growing back into the codebase again.

If you expect most PRs to be compliant, why are you against an automatic PR lint check then?

Instead, if this is possible, this should be done via Android Studio while typing the code.

I agree that this would be the best case. Unfortunately, while there are Gradle plugins, I couldn't get them to run automatically during coding or building. They seem to require manual configuration for each user.
However, Android Studio's code style settings are now checked into git, so just formatting the code inside Android Studio now fixes most lint issues.

[westnordost]

What I definitely don't want, and I think Mateusz mentioned it before, is some automatic lint check on GitHub, that prompts users to change their formatting when they create a PR, as seen here.

Why not? Do you think it's intimidating for new contributors? Or that it's a waste of time for them?

Yes, both. And a waste of time for the reviewers to point to. The linter is not even about catching possible issues, but about code style (spaces after commas and so forth), it is really unimportant.

If you expect most PRs to be compliant, why are you against an automatic PR lint check then?

I expect them to be generally compliant with the code style because people contributing tend to copy the style the find.

However, Android Studio's code style settings are now checked into git, so just formatting the code inside Android Studio now fixes most lint issues.

Well, that sounds good. Let's do that then. Is this already on master?

[FloEdelmann]

Yes, both. And a waste of time for the reviewers to point to. The linter is not even about catching possible issues, but about code style (spaces after commas and so forth), it is really unimportant.

Alright, then let's not include the PR lint check for now. I'd still add the check on the master branch though, as it will only cause a red cross ❌ on a failing commit there. That's nothing a non-regular contributor is likely to see.

Maybe this can be discussed again if/when there are more elaborate code smell lint checks.

Well, that sounds good. Let's do that then. Is this already on master?

No. I'll cherry-pick that from this PR and open a new PR.

[FloEdelmann]

I'll close this PR now, and create separate a PR that fixes the accepted lint rules.