-
Notifications
You must be signed in to change notification settings - Fork 746
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
Generalize -XepPatchChecks
command line parsing
#787
base: master
Are you sure you want to change the base?
Conversation
ScannerSupplier scannerSupplier, | ||
ErrorProneOptions epOptions, | ||
Context context, | ||
boolean forceFilter) { |
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 forceFilter
flag makes sure that the default patch mode of applying all built-in checks is disabled when a Refaster rule is supplied. I added this for backwards compatibility, but it does mean there's (still) no way to apply all built-in checks and any additional Refaster rules. That's another way in which Refaster is "special" (contrast this with service-loaded custom plugins: those are applied by default).
This is another thing I'd like to improve; input welcome!
3caecde
to
8da3686
Compare
8da3686
to
a74a436
Compare
Rebased the branch and resolved a conflict. What does the Error Prone team think of this approach? |
@Stephan202 Is this patch also available in the forked v2.3.4-picnic-2 release? I tried specifying the path to multiple .refaster check files, but the compiler then failed. |
@knutwannheden based on the code I'd say that should work, yes. What error do you see? NB: there's a possibility that this indeed doesn't work, because I don't think combining multiple Refaster templates like this has been tested recently. While (I believe a longer term fix entails a more user-friendly integration as described under the second bullet point in #552 (comment), but I'll admit that while this works, I'm no closer to polishing and open-sourcing that code. Chronic lack of time.) |
With my code being compiled through Maven I didn't really see any helpful error message at all, just some very generic message. I can report that back here, if you think it helps. Or maybe you know how I can get Maven to print the root cause here. |
Shot in the dark: add |
The error message wasn't displayed because of the So the error I get is <arg>-Xplugin:ErrorProne -XepPatchChecks:refaster:/path/to/first.refaster,refaster:/path/to/second.refaster -XepPatchLocation:${basedir}</arg> |
Okay, I stand corrected. This changeset is not included in the fork. 🤦♂️ And then the error message makes perfect sense. Apologies! Can you try to compile your Refaster rules in one go, using the changes from #552? (Those are included in the fork, I double-checked now 😅.) Otherwise I can try to create a new release that does include this change, but it might take a bit before I have time for that. |
Thanks for the heads-up. For my use case (part of jOOQ, which I think you are familiar with 😄) I actually want to have separate But since we are only in an exploratory phase, it isn't yet important to have the ability to use multiple |
Cool! jOOQ Refaster templates... we're definitely interested in those! (Say hi to @lukaseder :) ) (Going off-topic big time here, but the idea that library authors provide migration or best-practice Refaster rules for their libraries is one of the motivations behind the stuff written under the second bullet point in #552 (comment).) |
Hi @Stephan202 |
- Support arguments combining built-in and Refaster rules. - Support arguments listing multiple Refaster rules. Fixes google#595
a74a436
to
79c0363
Compare
Hi @Stephan202, Thanks for the PR! I was taking a fresh look at this, it seems like a nice thing to support. I was trying to think a bit about how this interacts with #947 and #4028, I wonder if we should decide what the semantics should be for #4028 first and then figure out how to make the refaster patch options consistent with that. |
This is a minimal (?) fix for #595. It adds support for:
-XepPatchChecks
arguments combining built-in and Refaster rules.-XepPatchChecks
arguments listing multiple Refaster rules.Open points and questions:
ErrorProneOptions
andBaseErrorProneJavaCompiler
; a nicer solution would entail making Refaster a first-class citizen. That'd be more work, but if you provide some pointers on the desired "end goal" or boundaries of the design space, I'm willing to have a crack at that, too. (In the context of a separate PR, ideally.)