Conversation
|
Should we run it now for the complete project? |
|
It is run already (see second commit), but the two cases that cannot be fixed (pr desc.) have to be fixed manually. Also, shall we get rid of detekt as well? |
|
Ok, thought it wasn't since those seemed like too few changes for such a big project, but looks like you tweaked the linter config to match the style we already had. Yes, I'd get rid of detekt here if it's not too much to ask. I don't think there's much value on keeping both of them. |
|
@JorgeCastilloPrz Only found detekt's config file, any pointer for the gradle one? |
JorgeCastilloPrz
left a comment
There was a problem hiding this comment.
Looks like there's not a Gradle one, thought we had some detekt related tasks somewhere but can't find it anymore
|
Pls remember to fix the reported style issues for Ank |
|
@JorgeCastilloPrz The reported files affect all entire project (see The question is ❓, do we ignore the issue file by file or fix it file by file too? I just need some hints as I'm unsure about if, for example, using wildcard imports is something you folks want to keep using or you'd be fine using qualified ones. Same question for the other issue described in the PR description. Thoughts? cc @pakoito @raulraja @nomisRev Details |
|
Wildcard imports are useful for importing complete namespaces so I'm fine keeping those. There's a trend on avoiding them but I don't think it would bring much value for us, since we intentionally group features per namespaces (i.e: arrow.effects) precisely to be able to use wildcard imports over those on call site files. |
|
Giving 99% of user base is in IDEA and IDEA manages imports for you I'm fine removing wildcard imports. IDEA auto import clean up already does so wherever it makes sense. Unless we are taking here about a collision between IDEA and ktlint expectations I think we should follow ktlint defaults to avoid friction as much as possible. |
|
As for file names that do not correspond to classes we should also follow ktlint expectations and have those files renamed as expected. |
|
It seems this PR is forcing new lines at the end of file to be removed due to the ktlint config used. Is that what we want? It feels a bit odd that the last line is not completed by a newline. |
|
Ensuring new line at the end of file is even a recommended rule to enable in ktlint documentation: https://github.com/pinterest/ktlint#standard-rules |
|
@raulraja 👌 on it. It seems ktlint had a fail-fast kind of mechanism, so now there come way more changes than before 😅 |
|
@aballano this is great, let me know when it's ready to be merged or merge at will whenever. thanks 👏 👏 |
| [*.{kt,kts}] | ||
| indent_size=2 | ||
| insert_final_newline=true | ||
| max_line_length=off No newline at end of file |
There was a problem hiding this comment.
Maybe also end this file with newline?
|
🎉 |
What's done
How to use
For verification is enough by doing
gradle buildor more explicitlygradle ktlint.For auto-correction:
gradle ktlintformatand it will fix most issues except some that cannot be auto corrected.What's missing
There are 2 issues that cannot be auto corrected and have to be fixed manually within this PR scope. The issues are:
Those doesn't seem to be configurable, so I'd rather prefer to hear opinions before changing them myself cc @arrow-kt/maintainers
=> The solution would be either ignore this issues file by file or fix them and prescind of wildcard imports and align file names with the class inside.