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

disable Modernizer via @SuppressWarnings annotation #3

Closed
gaul opened this issue Sep 18, 2014 · 14 comments · Fixed by #86
Closed

disable Modernizer via @SuppressWarnings annotation #3

gaul opened this issue Sep 18, 2014 · 14 comments · Fixed by #86

Comments

@gaul
Copy link
Owner

gaul commented Sep 18, 2014

@SuppressWarnings allows finer-grained suppression of violations than the existing file-based mechanism.

@ankon
Copy link
Contributor

ankon commented Feb 19, 2015

If I understand modernizer correctly, this wouldn't work: @SuppressWarnings is only retained in the source, not in the generated class files.

@gaul
Copy link
Owner Author

gaul commented Feb 19, 2015

@ankon I have limited background in annotation processing but I believe one can use the RUNTIME retention policy:

http://docs.oracle.com/javase/7/docs/api/java/lang/annotation/RetentionPolicy.html

@Stephan202
Copy link
Contributor

@andrewgaul, that's true when one has control over the annotation. In this case, however, java.lang.SuppressWarnings is annotated @Retention(RetentionPolicy.SOURCE). Not much we can do about that :/

@gaul
Copy link
Owner Author

gaul commented Feb 19, 2015

I see, so we would need a new SuppressModernizerWarnings class with the RUNTIME policy?

@Stephan202
Copy link
Contributor

That would be a solution, yes. (Though not quite as nice as the original suggestion.) Have no better idea at this point. (Other than adding support for source code analysis to the plugin, of course!)

@petteyg
Copy link

petteyg commented Feb 19, 2015

asm is operating on the bytecode, so I would think RententionPolicy.CLASS should be sufficient.

@Stephan202
Copy link
Contributor

Ah, right. That's more appropriate indeed.

@ankon
Copy link
Contributor

ankon commented Feb 19, 2015

Thinking a bit outside of the box here: One "evil" way could be to add a annotation processing step, which finds the @SuppressWarnings that pertain to modernizer, and dumps those into an exclusion file which is then used by the actual modernizer goal.

This would avoid polluting sources with build-system specific things ... except that of course the value in the annotation is also modernizer-specific, and in fact eclipse warns about unrecognized values in it :/

@vorburger
Copy link
Contributor

way could be to add a annotation processing step, which finds the @SuppressWarnings that pertain to modernizer, and dumps those into an exclusion file which is then used by the actual modernizer goal.

This would be cool, and help with #28 as well...

@keith-turner
Copy link

Findbugs has its own annotation for suppressing warnings. One interesting thing finbugs did is not caring which package the annotation is in, see : https://stackoverflow.com/a/44947252 . This allows users to declare the annotation and avoid having a dependency on findbugs.

@jhaber
Copy link

jhaber commented Feb 27, 2018

Just a heads up we're going to be working on this in our fork (https://github.com/HubSpot/modernizer-maven-plugin) and will upstream the changes once they're ready.

@gaul
Copy link
Owner Author

gaul commented Mar 21, 2018

@jhaber Any updates on this?

@jhaber
Copy link

jhaber commented Mar 21, 2018

We have it working where you can put @SuppressWarnings("modernizer") at the class level and that will get respected, and there is a PR to support this at the method level: HubSpot#2

Once that is merged, we were thinking that it would be a good MVP of this feature and we would send a PR to upstream those changes.

Some notes about our approach:

  • it depends on an annotation processor, which means that users need to add a new dependency for these suppressions to work (using <scope>provided</scope> on this dependency is fine, so it doesn't end up on your runtime classpath)
  • the annotation processor writes a CSV containing information about the excluded classes/methods which the plugin reads. We weren't sure if you had a preference for using something more structured like XML/JSON/YAML (the latter two would require additional dependencies in the processor/plugin)
  • @SuppressWarnings("modernizer") will disable all modernizer checks for the relevant code block. To support more granular suppressions, we would probably need to expand the violations file schema to include support for specifying an identifier for each violation. For example, the com/google/common/base/Function violation could have <id>GuavaFunction</id> and then you could do @SuppressWarnings("modernizer:GuavaFunction") to only suppress that specific check (this matches the syntax that other tools like checkstyle use to suppress specific checks, for example)

@gaul
Copy link
Owner Author

gaul commented Mar 24, 2018

Sounds good. I have concerns about the large number of spurious changes in that pull request so please minimize the noise before submitting a pull request. I look forward to this feature!

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

Successfully merging a pull request may close this issue.

7 participants