-
Notifications
You must be signed in to change notification settings - Fork 737
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
Exempt Immutable
from UnnecessarilyFullyQualified
#2236
Exempt Immutable
from UnnecessarilyFullyQualified
#2236
Conversation
Thanks for the PR, and sorry for the very slow response. In our codebase we're gradually moving towards standardizing on I also see the benefit of skipping them for projects that are mixing different I see a couple ways forward here:
What do you think? |
No worries. Disclaimer: I created this issue over two years ago and am trying to remember the context. Also, I work for a different company, where we don't have this problem.
I am not sure I understand the proposal here. Using |
I can report that the original company still has this issue. 😉 I guess an IIUC this way one could set |
You're right--I was thinking about ways to avoid a clash with another
That alternative SGTM, as long as |
Sorry for the delay; I created #4228 for the alternative approach Stephan suggested. (Created a separate PR because I don't think I have write access to this branch anymore) |
Following the discussions in #2236, this PR introduces the `BadImport:BadEnclosingTypes` flag to disallow nested types of specified enclosing type. This PR also takes the flag into account in `UnnecessarilyFullyQualified` to suggest importing `EnclosingType.NestedType` when possible. For instance, when `-XepOpt:BadImport:BadEnclosingTypes=org.immutables.value.Value` is set, it would suggest the following: ```java final class Test { @org.immutables.value.Value.Immutable abstract class AbstractType {} } ``` -> ```java final class Test { @Value.Immutable abstract class AbstractType {} } ``` Also, it won't require suppressing `UnnecessarilyFullyQualified` in the example in #2236. ```java import org.springframework.beans.factory.annotation.Value; final class Test { Demo(@value("some.property") String property) {} @org.immutables.value.Value.Immutable abstract class AbstractType {} } ``` Closes #2236. Fixes #4228 FUTURE_COPYBARA_INTEGRATE_REVIEW=#4228 from hisener:halil.sener/bad-import-bad-enclosing-types 67aa36d PiperOrigin-RevId: 595179563
Following the discussions in #2236, this PR introduces the `BadImport:BadEnclosingTypes` flag to disallow nested types of specified enclosing type. This PR also takes the flag into account in `UnnecessarilyFullyQualified` to suggest importing `EnclosingType.NestedType` when possible. For instance, when `-XepOpt:BadImport:BadEnclosingTypes=org.immutables.value.Value` is set, it would suggest the following: ```java final class Test { @org.immutables.value.Value.Immutable abstract class AbstractType {} } ``` -> ```java final class Test { @Value.Immutable abstract class AbstractType {} } ``` Also, it won't require suppressing `UnnecessarilyFullyQualified` in the example in #2236. ```java import org.springframework.beans.factory.annotation.Value; final class Test { Demo(@value("some.property") String property) {} @org.immutables.value.Value.Immutable abstract class AbstractType {} } ``` Closes #2236. Fixes #4228 FUTURE_COPYBARA_INTEGRATE_REVIEW=#4228 from hisener:halil.sener/bad-import-bad-enclosing-types 67aa36d PiperOrigin-RevId: 595179563
UnnecessarilyFullyQualified
flags the usage of fully qualified@Value.Immutable
annotation from Immutables. We prefer the fully qualified name when@Value
fromspring-beans
is used. So we need to suppress it in those cases.The following report is generated without the suppression:
However, using
@Immutable
can be confusing as there are many@Immutable
annotations, e.g.javax.annotation.concurrent.Immutable
,com.google.errorprone.annotations.Immutable
,org.hibernate.annotations.Immutable
, etc. For that reason, I think it makes sense to addImmutable
to exempted names.