-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Issue #13772: IllegalImport with class pattern fails on static import #14041
base: master
Are you sure you want to change the base?
Conversation
@@ -139,7 +139,7 @@ public final void setIllegalPkgs(String... from) { | |||
illegalPkgs = from.clone(); | |||
illegalPkgsRegexps.clear(); | |||
for (String illegalPkg : illegalPkgs) { | |||
illegalPkgsRegexps.add(CommonUtil.createPattern("^" + illegalPkg + "\\..*")); | |||
illegalPkgsRegexps.add(CommonUtil.createPattern("^(static|" + illegalPkg + ")\\..*")); |
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.
This is not much different from #13960 (comment) . This now looks like it can ignore the illegal package definition completely for the word static
. Even if it can't be valid java to show that issue, this still doesn't seem right.
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.
Few things here:
- The changes in this PR will have no effect on the behavior of this check unless we literally have a package name that contains
static
- Issue was originally for
illegalClasses
, but for some reason the regexp forillegalPkgs
is changed - I have removed approved label, since I do not see any issue with the current behavior of the check when compared to it's documentation.
If we were to do any work related to #13772, it would be to expand documentation to include some examples similar to what is in the issue description to make static import/method checking obvious.
I am blocking this PR until we have a resolution in #13772.
@nrmancuso, please consider #13772 (comment) |
I am removing my assignment here, we need to resolve discussion in issue and decide on what changes (if any) we need to make. Additionally, regardless of what we decide in the issue, the changes in this PR are not the direction we will be taking. |
Related: #13772