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

Open
fwilhe opened this Issue May 30, 2017 · 24 comments

Comments

Projects
None yet
@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

This comment has been minimized.

Show comment
Hide comment
@shyiko

shyiko May 30, 2017

Owner

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.

Owner

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

This comment has been minimized.

Show comment
Hide comment
@fwilhe

fwilhe May 30, 2017

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

Awesome, thanks for the prompt reply.

fwilhe commented May 30, 2017

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

Awesome, thanks for the prompt reply.

@asarazan

This comment has been minimized.

Show comment
Hide comment
@asarazan

asarazan 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.

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

This comment has been minimized.

Show comment
Hide comment
@shyiko

shyiko Jun 29, 2017

Owner

@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).

Owner

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

This comment has been minimized.

Show comment
Hide comment
@aem

aem 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

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

This comment has been minimized.

Show comment
Hide comment
@raffek

raffek commented Jul 13, 2017

+1

@fwilhe

This comment has been minimized.

Show comment
Hide comment
@fwilhe

fwilhe 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.

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

This comment has been minimized.

Show comment
Hide comment
@aem

aem 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

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

This comment has been minimized.

Show comment
Hide comment
@fwilhe

fwilhe 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.

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

This comment has been minimized.

Show comment
Hide comment
@aem

aem 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

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

This comment has been minimized.

Show comment
Hide comment
@fwilhe

fwilhe 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?

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

This comment has been minimized.

Show comment
Hide comment
@jackylimel

jackylimel 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?

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

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 12, 2017

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

ghost commented Sep 12, 2017

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

@snowe2010

This comment has been minimized.

Show comment
Hide comment
@snowe2010

snowe2010 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.

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

This comment has been minimized.

Show comment
Hide comment
@jackylimel

jackylimel Sep 21, 2017

I totally agree with you @snowe2010

jackylimel commented Sep 21, 2017

I totally agree with you @snowe2010

@BenjaminSchaaf

This comment has been minimized.

Show comment
Hide comment
@BenjaminSchaaf

BenjaminSchaaf 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.

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

This comment has been minimized.

Show comment
Hide comment
@henrik242

henrik242 Dec 20, 2017

Any progress?

henrik242 commented Dec 20, 2017

Any progress?

@Rosomack

This comment has been minimized.

Show comment
Hide comment
@Rosomack

Rosomack 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.

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

This comment has been minimized.

Show comment
Hide comment
@aseigneurin

aseigneurin 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?

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

This comment has been minimized.

Show comment
Hide comment
@aem

aem 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.

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

This comment has been minimized.

Show comment
Hide comment
@kenyee

kenyee 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

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

This comment has been minimized.

Show comment
Hide comment
@paramsen

paramsen 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.

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

This comment has been minimized.

Show comment
Hide comment
@shyiko

shyiko Apr 12, 2018

Owner

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

Owner

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

This comment has been minimized.

Show comment
Hide comment
@JacobMuchow

JacobMuchow 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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment