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

NF: activate no-wildcard-imports #12464

Closed
wants to merge 1 commit into from

Conversation

Arthur-Milchior
Copy link
Member

@Arthur-Milchior Arthur-Milchior commented Sep 20, 2022

This led me to realize some java.utils.FOO should be removed when FOO is also provided by Kotlin with better typing

Fixes #11852

@mikehardy
Copy link
Member

I discussed this previously. I no longer care about star imports, and the default configs are to allow them, I don't see why we should fight the default, as I don't see a benefit personally to non-star imports

#12047 (comment)


This is apparently a really contentious area.
I read through a couple threads, and this one (with a focus around here pinterest/ktlint#48 (comment)) was informative for me.
It appears that star imports are actually a more Kotlin-y thing (IntelliJ / Android Studio add them by default, the language implementation uses them, and the package is the "unit" of import in Kotlin more than a single file like Java)

So I'm leaning towards not caring about star imports, and ktlint in it's default configuration also does not care, so I'll let this go.

Note that I'm injecting opinion here, it's a matter of taste. Feel free to disagree. If most people disagree, it's possible to configure ktlint to disallow star imports from what I understand, and that's the thing to do so these rules are enforced by tooling not convention

@Arthur-Milchior
Copy link
Member Author

One point confuse me in your answer @mikehardy . We had to manually deactivate the lint rules because by default it rejects wildcard. So if we go with the default, I assume that we must not alter what seems to be the default.

When I let Android-Studio do whatever it wants when I press import, I see it sometime adds star and sometime removes them (it seems the rule is something such as "use start if there is at least five imports, otherwise use explicit import"), which leads to regular diff noise, and, recently, a question about why one star was added and another removed in the same commit (I guess one import became not needed and another one needed, but that's pure speculation)
This one should ensure canonicity.

More importantly, not importing java.utils.* will ensure that we each call to Foo will use Kotlin's FOO instead of java.utils.FOO, which is better typed. Admittedly, this can be solved by fordidding only java.utils.*

@mikehardy
Copy link
Member

Hmm - I was under the impression that the IDE (android studio) allowed them by default, and I didn't realize we differed from ktlint default by disabling. I'm inclined to go with the editor default to reduce friction. Motivation there being that I care little enough about the issue that reducing friction is weighty enough to sway me.

If there is really some potential for conflicting imports and the IDE does not complain about that or choose sensibly then perhaps it makes sense to disallow them, that's a strong argument. Does it really not complain if there's a name collision on import as you mention (java.util vs kotlin util ?)

@Arthur-Milchior
Copy link
Member Author

The only case I know of is that java.utils.* hides some types such as HashMap which are implicitly imported by Kotlin in every files. So if you import java.utils.* it hides Kotlin implementation silently, yes. But I could find a single example in all the codebase where it actually change nullability for HashMap (I've not yet tested for other types), so really, it would be a shameful argument in favour of this PR.

@oakkitten
Copy link
Contributor

Just here to say that I like this in general. Wildcard imports mean that if you are not in an IDE sometimes you can't Ctrl-F to see where this particular identifier came from. Also this leads to nosier diffs. No opinion on java.utils.*.

@Arthur-Milchior
Copy link
Member Author

Already 8 conflcts. Not surprised

@BrayanDSO
Copy link
Member

CI is broken

@BrayanDSO BrayanDSO added the Needs Author Reply Waiting for a reply from the original author label Sep 24, 2022
@github-actions
Copy link
Contributor

Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

@github-actions github-actions bot added the Stale label Nov 24, 2022
@github-actions github-actions bot closed this Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Author Reply Waiting for a reply from the original author Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove usage of Kotlin wildcard imports on the project Codestyle file
4 participants