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

Wildcard imports #48

Closed
fwilhe opened this issue May 30, 2017 · 57 comments
Closed

Wildcard imports #48

fwilhe opened this issue May 30, 2017 · 57 comments

Comments

@fwilhe
Copy link

@fwilhe fwilhe commented May 30, 2017

Please forgive my bikeshedding, but I beg to differ with the "no wildcards" rule. My primary argument is that intellij does add wildcard imports in the default configuration, and I think I should not have to configure intellij to play nice with ktlint. Adding // ktlint-disable no-wildcard-imports is a very ugly workaround.

Another acceptable solution for me would be if the format action actually resolved wildcard imports into individual imports, which it does not as of version 0.7.0. I don't want to argue about what's better, I just want to have a consistent style without requiring manual intervention.

@shyiko
Copy link
Collaborator

@shyiko shyiko commented May 30, 2017

Hi @fwilhe.

I presume you have read https://github.com/shyiko/ktlint#-with-intellij-idea (option 1).

Resolving * imports into individual imports is actually on my todo list. The thing is, it will require deeper integration with the compiler chain and it's hard to say how much time it's going to take. But, you (and anyone else bold enough to venture that way) are welcome to try it out. I'd happy to merge such PR in.

tldr; One way or another it's going to be done.

@fwilhe
Copy link
Author

@fwilhe fwilhe commented May 30, 2017

tldr; One way or another it's going to be done.

Awesome, thanks for the prompt reply.

@asarazan
Copy link

@asarazan asarazan commented Jun 27, 2017

Would it be possible to override this rule via the .editorconfig file? Seems like adding no-wildcard-imports = false would be a pretty elegant solution.

@shyiko
Copy link
Collaborator

@shyiko shyiko commented Jun 29, 2017

@aem Thank you for the PR. It looks ready to merge (as soon as we have reached some kind of consensus here).

@asarazan My concern is - EditorConfig isn't really supposed to deal with language-specific settings. I'm aware of https://github.com/editorconfig/editorconfig/wiki/editorconfig-properties but I'm not sure how I feel about it. On the one side adding this property feels like abusing .editoconfig and bike-shedding, on the other - it could reduce contention.

BTW: To my knowledge, Intellij does not support anything but indent_size, continuation_indent_size, tab_width, indent_style at the moment (note that all of them are editor-centric).

@aem
Copy link

@aem aem commented Jun 29, 2017

@shyiko I definitely agree that this feels a bit like fitting a square editorconfig into a round hole, not sure where else it would go though. i just went with editorconfig since it's what was used for the indents (which is admittedly the right use of editorconfig)

from our perspective, due to some very specific needs of our product we have enums with 15+ cases, importing each of those without wildcards introduces a lot of extra loc and makes diffs much longer/harder to read. we'd love an override here, regardless of what form it takes

@raffek
Copy link

@raffek raffek commented Jul 13, 2017

+1

@fwilhe
Copy link
Author

@fwilhe fwilhe commented Jul 18, 2017

I just looked at a few random sample projects in the official Kotlin group (https://github.com/Kotlin), and I saw no source file that did not use wildcard imports (ok, my sample size was small, but I don't think I just got the few exceptions). I don't think that there is consensus for this rule among the Kotlin devs, so I'd argue for just drop it.

@aem
Copy link

@aem aem commented Jul 18, 2017

@fwilhe i'd argue the exact opposite. some people prefer no wildcards, but the default IntelliJ config automatically adds them. forcing all consumers of ktlint who use the default IntelliJ config to change their IntelliJ config and then go through potentially 100s of Kotlin files to expand all of the imports seems to be bikeshedding at its best, so the alternative is to allow a 1-line config or single command-line flag override to allow us to get back to writing code ASAP

@fwilhe
Copy link
Author

@fwilhe fwilhe commented Jul 18, 2017

@aem I'm not sure if there is a misunderstanding. I don't really get what your position is.

The first feature that the readme of ktlint lists is "no configuration". And I value this feature. I think the one thing that Go really got right is gofmt. I value "no configuration" more then "consistency of import style". To be honest, I mostly don't see the imports because they are collapsed and managed by the IDE.

@aem
Copy link

@aem aem commented Jul 18, 2017

@fwilhe check the explanation in #43 when allowing 2 spaces. there's a precedent for value being assigned to this tool being a solution that doesn't require reformatting your whole codebase, even if it requires an extra 1 line of config. i don't see this as being too different from that

@fwilhe
Copy link
Author

@fwilhe fwilhe commented Jul 18, 2017

I'm aware of #43, but I don't think this justifies more configuration. Feels a bit like a "broken window" case. "We got one option, sure a second option won't hurt". What is so bad about not checking if there are wildcard imports?

@jackylimel
Copy link

@jackylimel jackylimel commented Aug 30, 2017

so if EditorConfig isn't really supposed to deal with language-specific settings, is it possible to have some configuration files, e.g. ktlint.config, which specifies the lint rules and the library read the configuration from it?

@ghost
Copy link

@ghost ghost commented Sep 12, 2017

+1 for @jackylimel 's suggestion of a ktlint.confg

@snowe2010
Copy link

@snowe2010 snowe2010 commented Sep 20, 2017

Yeah besides ktlint being unable to resolve the imports out (expand them) this choice does not make sense to me. Kotlin allows top level functions, which allows us to create test factories that only need to be imported as one wildcard import. If we expanded every single import our test files would triple in size just from import statements.

@jackylimel
Copy link

@jackylimel jackylimel commented Sep 21, 2017

I totally agree with you @snowe2010

@BenjaminSchaaf
Copy link

@BenjaminSchaaf BenjaminSchaaf commented Oct 26, 2017

As a datapoint, the source for kotlin contains over 6000 wildcard imports across 35303 kotlin source files. Personally I embrace wildcard imports because they increase readability.

@henrik242
Copy link

@henrik242 henrik242 commented Dec 20, 2017

Any progress?

@Rosomack
Copy link

@Rosomack Rosomack commented Jan 8, 2018

Just to drop my 2 cents here:

I don't get why wildcard imports are considered to be evil. If they were, Kotlin would not copy them from Java - they seem to have dropped a lot of other baggage.

Also some inconsistencies:

  1. The kotlin coding conventions don't even mention imports.
  2. Jetbrains' yole opinion is that imports are for the IDE to manage
  3. The only place where import guidelines exist is the google style guide

Since this linter is based on both style guides, there is a conflict here and I'd personally err towards abandoning that check. and leaving it for the IDE to manage.

@aseigneurin
Copy link

@aseigneurin aseigneurin commented Jan 24, 2018

Although I can understand that wildcard imports can be a source of problems, in my experience programming with Scala, Java and Kotlin, I have actually never seen any compilation problem coming from these imports.

Also, IntelliJ IDEA has become a standard in our industry and it has a pretty decent default configuration. I am using Ktlint to make sure that developers don't push code that hasn't been formatted to this standard, not to apply a different set of rules.

So, my take is that Ktlint should - by default or through some configuration - apply the rules that come by default with IntelliJ IDEA, and it seems that most people who contributed to this thread came to the same conclusion.

I think the idea of a ktlint.config file (or whatever its name is) is great, and since we're all developers, we could all contribute to this addition to Ktlint.

Obviously, this is not line with your original idea of the tool, @shyiko, so I guess the key question is: if we made a PR to allow configuring the rules, would you be open to accepting this PR?

@aem
Copy link

@aem aem commented Jan 24, 2018

@aseigneurin At the point at which that idea is accepted, I would suggest creating a fork of this project and publishing it under a different name. While I am in favor of wildcard imports, a config file seems to be antithetical to this project (aiming to be anti-bikeshedding) and thus a PR to do that would be better merged into a separate fork rather than this project. I'm all in favor of my PR being merged, but I don't think it makes sense to merge it or shoehorn it into the project just because it's there. If it's decided that it doesn't fit into this project then there's no need to force it.

@kenyee
Copy link

@kenyee kenyee commented Apr 5, 2018

I'd actually suggest disabling this rule by default unless you can match the #imports before Android Studio automatically uses a wildcard.
This is one of the pain points when I first started using ktlint

@paramsen
Copy link

@paramsen paramsen commented Apr 12, 2018

So this is a rule that isn't mentioned in the guidelines that the linter is said to implement; and the only argument against allowing a config flag for disabling said rule is that someone added the term anti-bikeshedding in the readme?

Seems like the readme should be altered a bit: "capture (reflect) official code style from kotlinlang.org and Android Kotlin Style Guide and my own personal shoot-from-the-hip formatting preferences."

Sorry, my bad, I based my conclusion on other comments; The guide lines does specify explicitly that wild card imports are to be avoided.

@shyiko
Copy link
Collaborator

@shyiko shyiko commented Apr 12, 2018

@paramsen
From the official Android Kotlin Style guide, Import statements section:
"Wildcard imports (of any type) are not allowed."

@JacobMuchow
Copy link

@JacobMuchow JacobMuchow commented Apr 13, 2018

@shyiko I am interested in this issue because like others I think the linter should work with the default import behavior of the IDE. I don't really want to make my whole development team go through the IDE steps to make it work with the linter. However, I also agree the linter should follow the official style guide...

I made an issue in the Android Kotlin Style Guide repo to change this rule if anyone would like to give it support. If the guide is updated, then we can update the linter and everything can work in unison.

android/kotlin-guides#65

@Zhuinden
Copy link

@Zhuinden Zhuinden commented Dec 3, 2018

Apparently they really don't care that this is a blocker for many people (as indicated by how long this issue has been open for along with the "what are your blockers to start using ktlint"), maybe someone should just create a fork and remove the rule altogether then publish it for public use.

@shashachu
Copy link
Contributor

@shashachu shashachu commented May 22, 2019

@snowe2010 is the JetBrain standard you're referencing this one? https://ktor.io/quickstart/code-style.html

@shashachu
Copy link
Contributor

@shashachu shashachu commented May 22, 2019

My general opinion is that if this is actually preventing widespread adoption of this tool, the rule should be configurable.

In a pre-1.0 tool, I'm okay with short-term, non-ideal solutions, with the understanding that they may get pulled/broken at any time. The long-term solution will likely be something akin to a .ktlintignore or .ktlintconfig file where we can disable rules on a per-path basis.

In the short term, I see a few pretty simple solutions:

  1. Pull Wildcard imports rule from standard ruleset - projects wanting that rule can bring it in as a custom rule
  2. Resurrect #61, acknowledging that it's kind of abusing .editorconfig.
  3. Add it to command line options, per this comment #48 (comment)
@snowe2010
Copy link

@snowe2010 snowe2010 commented May 22, 2019

@JakeWharton they're really not. Java has no concept of top-level functions, that by itself is a fantastic reason to always allow them in Kotlin, but not in Java.

@shashachu I'm referring to the Editor > Code Style > Kotlin > Set From > Predefined Style > Kotlin Style Guide setting in IntelliJ. I do not know how well this matches up with the ktor guide, though I know they're supposed to be the same thing.

@JakeWharton
Copy link
Contributor

@JakeWharton JakeWharton commented May 22, 2019

Java has static imports of functions which behave the exact same way. So no.

@snowe2010
Copy link

@snowe2010 snowe2010 commented May 22, 2019

And static imports are not used in nearly the wide variety of circumstances that top-level functions are. We converted over 500k lines of Java code straight to kotlin. Switching from wildcard imports to non-wildcards ballooned each file by about a hundred lines, depending on the context, due to the number of uses of top-level functions. They're not the same thing, no matter how much they act like it.

@JakeWharton
Copy link
Contributor

@JakeWharton JakeWharton commented May 22, 2019

Good thing source file lines are free, import lists are collapsed automatically, and the list is managed automatically by the IDE. What are you optimizing for?

@snowe2010
Copy link

@snowe2010 snowe2010 commented May 22, 2019

If you don't care about it then why are you here arguing. I do care, and my team cares. This tool isn't following the standard set by the makers of the language, and I think it should be changed. In response to your questions:

  • source lines aren't free, every additional line is another line for Sonar to manage and more for us to pay for, especially at the rate of 100 lines per file over tens of thousands of files
  • import lists still need to be read through at some point, the shorter the better.
  • IDEs aren't perfect, and IntelliJ manages to mess up non-wildcard imports all the time.
@shashachu
Copy link
Contributor

@shashachu shashachu commented May 22, 2019

For the record, I agree with the rule (I don't like wildcard imports), but from a ktlint adoption standpoint, making it optional is the right thing to do. It seems like this is a hard blocker for many projects.

@snowe2010
Copy link

@snowe2010 snowe2010 commented May 22, 2019

I agree that these things should be configurable. One of my favorite libraries is rubocop and every single thing is configurable. I believe ktlint should be the same, if the maintainers want a wide adoption.

@JakeWharton
Copy link
Contributor

@JakeWharton JakeWharton commented May 22, 2019

source lines aren't free, every additional line is another line for Sonar to manage and more for us to pay for, especially at the rate of 100 lines per file over tens of thousands of files

LOL what a dumb tool

import lists still need to be read through at some point, the shorter the better.

What information are you intending to glean from reading a wildcard import that is better served than individual imports?

IDEs aren't perfect, and IntelliJ manages to mess up non-wildcard imports all the time.

IDE bugs are orthogonal and can be simply be filed and fixed. You aren't changing style guides based on other IDE bugs, are you? That sounds pretty devastating. IDE can't see a function on a type? Better disable auto-complete.

@shashachu
Copy link
Contributor

@shashachu shashachu commented May 22, 2019

Hi all, this is clearly a topic that developers feel really passionate about; let's just remember to keep comments constructive and relevant.

Back to the issue at hand, the simplest short term solution will be to pull this rule from the standard ruleset. We'll leave the code in the repo so that it will be easy for developers who wish to enable it for their projects can bring it in as a custom rule.

Once that is merged, there are a couple more small bugfix PRs I'd like to merge, and then we'll make a new release. (May not be until after the Memorial Day holiday.)

Thanks everyone for the input!

@shashachu
Copy link
Contributor

@shashachu shashachu commented May 22, 2019

Using kotlin-android-extensions (for the synthetic view accessors) without wildcard imports is just pain.

FWIW, kotlin android extensions are explicitly allowed:

if (path != null && !path.startsWith("kotlinx.android.synthetic") && path.contains('*')) {
    emit(node.startOffset, "Wildcard import", false)
}
shashachu added a commit to shashachu/ktlint that referenced this issue May 22, 2019
shashachu added a commit that referenced this issue May 22, 2019
Bandaid for #48

Also adding list of disabled rules to README for visibility.
@shashachu
Copy link
Contributor

@shashachu shashachu commented May 22, 2019

Closing this, as the issue has reached temporary resolution.

@shashachu shashachu closed this May 22, 2019
@PandaGeek1024
Copy link

@PandaGeek1024 PandaGeek1024 commented May 26, 2019

@shashachu Appreciate your great effort. Would you mind to share how to add the wildcard rules back when needed? is it through Creating a ruleset? thanks.

@shashachu
Copy link
Contributor

@shashachu shashachu commented May 26, 2019

@PandaGeek1024 that's correct - you'd copy in the code for it as a custom rule. Check out the Creating a ruleset section of README.md and feel free to open up an issue if you're having trouble.

@JLLeitschuh
Copy link
Contributor

@JLLeitschuh JLLeitschuh commented May 28, 2019

How can we enable this at runtime with the new update? I'd like to be able to update the Ktlint Gradle and Spotless documentation on how to keep the same functionality if you want it.

@Zhuinden
Copy link

@Zhuinden Zhuinden commented May 28, 2019

Create a custom ruleset as per https://github.com/pinterest/ktlint#creating-a-ruleset which will contain a rule that is literally a copy paste of the code in

override fun visit(
node: ASTNode,
autoCorrect: Boolean,
emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit
) {
if (node.elementType == IMPORT_DIRECTIVE) {
val importDirective = node.psi as KtImportDirective
val path = importDirective.importPath?.pathStr
if (path != null && !path.startsWith("kotlinx.android.synthetic") && path.contains('*')) {
emit(node.startOffset, "Wildcard import", false)
}
}
}

@idntfy
Copy link

@idntfy idntfy commented Jul 16, 2019

In this issue everyone agreed that the rule should be disabled. And guess what? I see it was enabled back in the 0.34.0 release (released 2 days ago) without any references and discussions in the release notes - it's just mentioned that it was enabled back without any "why". Shall this issue be reopened?

@shashachu
Copy link
Contributor

@shashachu shashachu commented Jul 16, 2019

@idntfy actually it is specifically called out in the release notes for 0.34.0: https://github.com/pinterest/ktlint/releases

The agreement was to temporarily disable the rule until there was support for globally disabling a rule, which was added in 0.34.0: https://github.com/pinterest/ktlint#custom-editorconfig-properties

@idntfy
Copy link

@idntfy idntfy commented Jul 16, 2019

Surprising! I have more belief that the comment where the agreement was made (?) was just misread and some sort of miscommunication happened, because for me it seems there were much more arguments for not having it than enforcing. I'm not going to repeat all the good arguments that were already said in this issue.

The README.MD says "No configuration. Which means no decisions to make, nothing to argue about and no special files to manage. While this might sound extreme, keep in mind that ktlint tries to capture (reflect) official code style" which is now not true anymore, because to get to widespread defaults now we need to configure it.

On a scale it's really hard to adopt now when it's enabled. We have both big number of projects and even more bigger number of developers. Committing .editorconfig to all the projects or asking everyone to call --apply-to-idea-project are just no way to go, because it cannot be centralized thus hardly manageable.

The either acceptable way to make it centralized would be to provide the way to configure it through cli params, so it could be done just in one place in a parent pom lets say.

@idntfy
Copy link

@idntfy idntfy commented Jul 17, 2019

Created #533 as an attempt to find a pragmatic consensus.

@shashachu
Copy link
Contributor

@shashachu shashachu commented Jul 17, 2019

Surprising! I have more belief that the comment where the agreement was made (?) was just misread and some sort of miscommunication happened, because for me it seems there were much more arguments for not having it than enforcing. I'm not going to repeat all the good arguments that were already said in this issue.

The README.MD says "No configuration. Which means no decisions to make, nothing to argue about and no special files to manage. While this might sound extreme, keep in mind that ktlint tries to capture (reflect) official code style" which is now not true anymore, because to get to widespread defaults now we need to configure it.

On a scale it's really hard to adopt now when it's enabled. We have both big number of projects and even more bigger number of developers. Committing .editorconfig to all the projects or asking everyone to call --apply-to-idea-project are just no way to go, because it cannot be centralized thus hardly manageable.

The either acceptable way to make it centralized would be to provide the way to configure it through cli params, so it could be done just in one place in a parent pom lets say.

Sorry this came as a surprise to you; disabling the rule in 0.33.0 was always intended to be a temporary solution. I actually agree with you that we're straying from the 'No configuration' tenet, but the wider the adoption of the tool, the more difficult it will become that everyone agrees on every rule. When we took over the project, Stanley said it was always his intention to support some way to globally disable rules.

akuleshov7 pushed a commit to cqfn/diKTat that referenced this issue Jun 29, 2020
Bandaid for pinterest/ktlint#48

Also adding list of disabled rules to README for visibility.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.