Skip to content
This repository has been archived by the owner on Oct 16, 2018. It is now read-only.

Allow wildcard imports #65

Open
JacobMuchow opened this issue Apr 13, 2018 · 11 comments
Open

Allow wildcard imports #65

JacobMuchow opened this issue Apr 13, 2018 · 11 comments

Comments

@JacobMuchow
Copy link

JacobMuchow commented Apr 13, 2018

https://android.github.io/kotlin-guides/style.html#import-statements

Wildcard imports (of any type) are not allowed.

This rule seems contradictory to currently accepted standards for general Kotlin and Kotlin w/ Android development. To borrow from what @Rosomack and others said in this ktlint issue about wildcard imports:

  1. Kotlin coding conventions never mention rules for imports. Also, it reasons to stand that usage of wildcard imports should be by coder-preference given that the language allows them. Why should the Android Kotlin style guide have rules about imports when Kotlin does not?

  2. According to @BenjaminSchaaf, the source code for Kotlin itself contains over 6000 wildcard imports across 35303 Kotlin source files.

  3. ItelliJ / Android Studio use wildcard imports by default in these cases:

  • When more than 5 names are used from the same file.
  • When importing classes form java.util.* (ex. LinkedList)
  • When importing synthetic properties generated by the Kotlin Android extension. ex)
import kotlinx.android.synthetic.main.activity_main.*

Note that this is specific behavior set up by the plugin, not because of > 5 names being used.


Most Kotlin linters (such as ktlint) are based off this style guide. Because of this rule, programmers are having to go out of their way to change the default behavior of the IDE to stop using wildcard imports altogether. I propose this rule be removed from the guide.

@shyiko
Copy link

shyiko commented Apr 13, 2018

Personally, I'd advise against this.

Quoting checkstyle AvoidStarImportCheck
"Importing all classes from a package or static members from a class leads to tight coupling between packages or classes and might lead to problems when a new version of a library introduces name clashes."

In addition to the above, wildcard imports make it harder to navigate through the code outside the confines of IDE (e.g. while browsing code on GitHub/GitLab/etc). They also complicate matters for static analysis tools as it becomes impossible to say where something is coming from without building and checking the classpath. So don't count on OctoLinker (or anything similar) to help you traverse wildcard imports in future.

This change would also be in contradiction with Google Java Style Guide - 3.3.1 No wildcard imports.

It would be great if Jetbrains could be persuaded to disallow wildcard imports by default but I'm not sure how feasible that is.

@NightlyNexus
Copy link

Nobody should be importing java.util.LinkedList lol.

@BenjaminSchaaf
Copy link

@shyiko I use sublime as my daily driver and by reading the imports I can get a quick overview of what the code is using. I find it much easier to get this overview with wildcard imports. The same goes when viewing the code through github or similar.

Being greeted with a wall of imports for every new file I look at is not something I find at all useful. I'm much more interested what modules you're using.

The last time I had a name clash was with libgdx's Array class (same name as the kotlin builtin class). Now every time I view that file that specific import sticks out exactly like it should and it is much more obvious that said class is now available as GdxArray instead of the default Array. If I instead had the 100 lines or so of libgdx imports that specific one would probably be overlooked most of the time.

@JacobMuchow
Copy link
Author

Whoops :p

@JakeWharton
Copy link
Contributor

I don't buy 1 as an argument since there's plenty of things we specify that they do not. I also don't really buy the language argument thing since it's likely just a leftover from originally being a JVM-only language. They probably should have been left out but it's likely a concession for people who don't use IDEs.

2 isn't a good argument because there's more 2-space indent than 4-space indent in their codebase and yet their guide and our guide requires 4-space indent. When you go looking for violations of rules that were defined years after a codebase was created you will always find them.

We have the power to fix 3 in AS. That's a simple bug.

The reason they are not allowed is that no one has made a compelling case for allowing them. Here's why I don't think they should be alowed:

  • The IDE manages type, function, and property imports for you and will automatically add missing ones both while auto-completing and also via alt+enter.
  • In the IDE, import lists are collapsed by default and thus vertically shortening the list has no actual effect on how far down in the editor source code actually starts.
  • Wildcard imports create the possibility of simple name collision requiring explicit import anyway.
  • Wildcard imports reduce clarity while performing code review or browsing source on the web.

Every argument I've ever heard in favor of wildcard imports has actually been a thinly-veiled IDE bug or feature request. If someone can legitimately argue for wildcard imports on the advantage they provide and that the advantage is greater than the disadvantages then I think the change is worth considering. I haven't seen any such argument yet.

@JakeWharton
Copy link
Contributor

It's also worth noting two things about what we provide here:

  1. We assume you are using Android Studio.
  2. These are guidelines and not rules.

@snowe2010
Copy link

For our use case we use many top-level kotlin functions for creating objects for tests. We use these 'factory' methods in our tests. Needing to import tens of objects for tests gets unwieldy especially due to the nature of top-level functions. It can triple the size of each test file just from imports.

Note, I have no dog in this fight, I'm here because @shyiko wants ktlint to match the Styleguide to some point.

@JakeWharton
Copy link
Contributor

Are you explicitly writing the imports or is the IDE automatically adding them for you as you type?

In what context are you concerned about the test file size? The IDE automatically hides the import block by default.

@snowe2010
Copy link

@JakeWharton The IDE usually adds them. I do see some issues sometimes with top-level functions if they are named the same as the variable I'm creating.

for example: this top level method

fun userId(static: Boolean = USE_STATIC_IDS): UUID =
        if (static) UUID.fromString("3a6d8b25-99ae-46ee-a149-712986c5f3ae") else UUID.randomUUID()

if I attempt to use it somewhat like this:

fun viewDao(
        ...
        userId: UserId = userId(),
        ...
) = ViewDao(..., userId, ...)

IntelliJ will see userId as a parameter and it won't attempt to import. If I already have a wildcard import it works fine.

@ZakTaccardi
Copy link

import kotlinx.android.synthetic.main.activity_main.*

This is the only wildcard import that should be considered being allowed.

We have the power to fix 3 in AS. That's a simple bug.

Is the bug in question the inability for AS to autocomplete a synthetic method without having the star import? If so, then this should be disallowed when it's fixed. Is there a issue tracking this anywhere?

Nobody should be importing java.util.LinkedList lol.

@NightlyNexus why does it matter? IDE autocompletes anyway

@breandan
Copy link

breandan commented Aug 4, 2018

Some feedback here: Yuri relies on wildcard imports in order for code completion to work properly. Since the Android Kotlin Style Guide explicitly forbids wildcard imports, Yuri is not compatible with style-guide compliant Android code. Furthermore, as per square/kotlinpoet#275, since the Android forbids wildcard imports, KotlinPoet explicitly prevents building code with wildcard imports. I vote for relaxing or removing this rule.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants