-
Notifications
You must be signed in to change notification settings - Fork 81
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
Keep imports from the same package if the name is overloaded #414
Conversation
@hick209 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
core/src/main/java/com/facebook/ktfmt/format/RedundantImportDetector.kt
Outdated
Show resolved
Hide resolved
val hasAlias = it.alias != null | ||
val isOverload = requireNotNull(identifierCounts[it.identifier]) > 1 | ||
// Remove if... | ||
!isUsed || (isFromThisPackage && !hasAlias && !isOverload) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might actually want to pivot this to just remove if it's unused, which keeping things from the same package always.
We have internal reasons to want this change, but this is something we likely will do in the future and if you state that this is not something you'd want we could put it under a config. Please let us know.
Things are fine for this PR as they are though, just to be clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do generally prefer the behaviour of removing imports for the local package, since it keeps things consistent. Otherwise, some local package elements will be imported, and others won't be, depending on the whims of various authors.
The case I'm trying to fix has to do with overload resolution for Truth. The issue seems to be that importing Truth.assertThat
causes kotlinc to always use an overload from the Truth
class, even when there's a local package function named assertThat
, which has narrower typing. Part of the problem here is there's a definition Truth.assertThat(x: Object)
which matches ~everything, which is a bad but entrenched design.
core/src/main/java/com/facebook/ktfmt/format/RedundantImportDetector.kt
Outdated
Show resolved
Hide resolved
@hick209 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
* upstream/main: Add unit tests to capture line break behavior on type specifiers Plugin doesn't work with if "Only VCS changed text" is selected from code-reformat settings (facebook#386) Bump version to 0.47-SNAPSHOT Bump version to 0.46 Fix indentation of trailing comments in a bunch of cases (facebook#418) Adjust .editorconfig for kotlinlang style for IntelliJ to better align with ktfmt (facebook#412) Bump Kotlin version to 1.8.22 Bump version to 0.46-SNAPSHOT Bump version to 0.45 Bump word-wrap from 1.2.3 to 1.2.4 in /website (facebook#410) Use inExpression in a nullsafe way (facebook#417) Update ktfmt component on FBS:master Back out "Improve argsfile support" Improve argsfile support Fix double indentation in Elvis chains (facebook#416) Daily `arc lint --take KTFMT` Remove TypeNameClassifier Support context receivers (facebook#400) Added link to live playground directly into README file Keep imports from the same package if the name is overloaded (facebook#414)
No description provided.