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

Document how to apply Refaster suggestions #598

Closed
lukelukes opened this issue Apr 26, 2023 · 9 comments
Closed

Document how to apply Refaster suggestions #598

lukelukes opened this issue Apr 26, 2023 · 9 comments

Comments

@lukelukes
Copy link

Problem

I couldn't find any documentation on how to apply the Refaster suggestions. The suggestions coming from Errorprone are applied as expected, but the Refaster ones don't and it is not obvious how to apply them. (Does it require your fork of Errorprone?)

@Stephan202
Copy link
Member

Hey @lukelukes! Thanks for reaching out. Indeed we can do better here.

To get you unblocked, it should be enough to:

  1. Add tech.picnic.error-prone-support:refaster-runner to annotation processor classpath. (This part is documented.)
  2. Make sure that the Refaster check is enabled (this should be the case by default, unless -XepDisableAllChecks or -XepDisableAllWarnings is specified).
  3. When patching using vanilla Error Prone, it's important that one specifies -XepPatchChecks:Refaster.

Let us know if this helps; we'll use the feedback to clarify the documentation!

@lukelukes
Copy link
Author

lukelukes commented Apr 26, 2023

Hey @Stephan202, thanks for the quick answer!
It's applying the suggestions now, but the regex provided in your documentation to filter out Refaster checks did not work with Gradle (haven't tried with Maven).

For some reason yours excludes all Refaster checks, however this works:

"-XepOpt:Refaster:NamePattern=^(?!.*(?:$ignoredPicnicRefasterRulesRegex)).*\$"

Where ignoredPicnicRefasterRulesRegex is just a String of all the ignored check names without their class (e.g ImmutableListOf instead of ImmutableListRules.ImmutableListOf etc.) joined by a | (regex OR).

@Stephan202
Copy link
Member

Ah, this is good feedback. I'll have to check, but I suspect that what's documented won't work for Maven either 😅.

I just ran this test in IDEA with a debugger and set a breakpoint on this lambda. The names against which it matches for this Refaster rule collection are:

  • FooRules$ExtraGrouping$StringOfSizeTwoRule
  • FooRules$ExtraGrouping$StringOfSizeThreeRule
  • FooRules$StringOfSizeOneRule
  • FooRules$StringOfSizeZeroRule
  • FooRules$StringOfSizeZeroVerboseRule

So I think the documentation should be updated to use \$instead of \. as a type separator. I'll test this on the command line and push a fix to the website branch once validated. Tnx for the report!

@Stephan202
Copy link
Member

Alright, I tested it with Maven and found that (a) the inner parens were unnecessary (for the single-rule exclusion case) and (b) a .* suffix was missing. The website is now updated. Thanks again for reporting!

If the new documentation works for you as well I'll close the issue :)

@originalrusyn
Copy link

Hi @Stephan202 ,
Could you please suggest how to configure Refaster to apply only specific rules?
I've tried to do it with
"-XepPatchChecks:Refaster:OptionalOrElseThrow", "-XepPatchLocation:IN_PLACE"
flags but it seems doesn't work

@Stephan202
Copy link
Member

Hey @originalrusyn! Try with e.g. "-XepOpt:Refaster:NamePattern=.*OptionalOrElseThrow" :)

@originalrusyn
Copy link

originalrusyn commented Jun 13, 2023

@Stephan202
Thanks,
It works for:

"-XepPatchChecks:Refaster", "-XepPatchLocation:IN_PLACE", "-XepOpt:Refaster:NamePattern=(OptionalRules.OptionalOrElseThrow|TimeRules.UtcConstant)"

So, I can only use regex like that to apply multiply Refaster rules at the same time?
And is it possible to configure Refaster to report about other possible refactoring (which isn't configured to refactor automatically) it found in code at the same time?

@Stephan202
Copy link
Member

Happy to hear it works!

So, I can only use regex like that to apply multiply Refaster rules at the same time?

For now the -XepOpt:Refaster:NamePattern regex option is indeed the only way to control what gets applied, indeed. In the context of #655 we'll look at alternatives, but I can't give a timeline for that right now.

And is it possible to configure Refaster to report about other possible refactoring (which isn't configured to refactor automatically) it found in code at the same time?

Ah you mean to use -XepPatchLocation:IN_PLACE and at the same time see warnings about other Refaster rule matches? I don't think so, because patching is handled by Error Prone based on what a bug checker (in this case Refaster) reports, without the bug checker being aware of this. I guess a workaround could be to run the compilation twice, with different configurations. (Generally, patching is only done on-demand, while warnings are more relevant during regular builds. So perhaps a second compilation round is an acceptable price to pay.)

@lukelukes
Copy link
Author

This is no longer an issue, thanks for the help.

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

No branches or pull requests

3 participants