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

Support running all available patch checks #947

Conversation

Stephan202
Copy link
Contributor

The BaseErrorProneJavaCompiler supports running all patch checks; it just requires that one doesn't provide any explicitly named checkers.

In practice this doesn't work, however, because:

  • PatchingOptions requires a named checker or a custom refactorer.
  • Specifying -XepPatchChecks: without any explicit check will cause the empty string to be stored as a named checker.

This PR proposes to resolve the above issues by:

  1. Requiring -XepPatchLocation when -XepPatchChecks is specified, but not vice versa.
  2. Omitting empty strings from -XepPatchChecks:.

Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Feb 21, 2018
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Apr 19, 2018
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Apr 25, 2018
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Oct 13, 2018
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Feb 24, 2019
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Dec 7, 2019
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request May 30, 2020
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947
@Stephan202 Stephan202 force-pushed the bugfix/support-empty-PatchChecks branch from 020d45e to ec1094f Compare January 16, 2021 14:55
@Stephan202
Copy link
Contributor Author

Rebased the branch; would still be interested in seeing this feature included upstream.

Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Jan 16, 2021
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Jan 16, 2021
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Apr 2, 2021
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request May 15, 2021
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request May 15, 2021
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Jul 23, 2021
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Aug 4, 2021
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Aug 11, 2021
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.

(cherry picked from commit fc91b0e)
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Aug 20, 2021
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Aug 30, 2021
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.

(cherry picked from commit fc91b0e)
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Nov 13, 2021
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
rickie pushed a commit to PicnicSupermarket/error-prone that referenced this pull request Dec 7, 2021
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.

(cherry picked from commit fc91b0e)
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Jan 26, 2022
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
rickie pushed a commit to PicnicSupermarket/error-prone that referenced this pull request Jan 31, 2022
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.

(cherry picked from commit fc91b0e)
rickie pushed a commit to PicnicSupermarket/error-prone that referenced this pull request Jan 31, 2022
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.

(cherry picked from commit fc91b0e)
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Apr 13, 2022
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Apr 13, 2022
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Apr 15, 2022
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Apr 16, 2022
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Jun 5, 2022
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Aug 4, 2022
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Oct 12, 2022
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Oct 13, 2022
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Dec 31, 2022
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.
@Stephan202 Stephan202 force-pushed the bugfix/support-empty-PatchChecks branch from ec1094f to c5d9125 Compare December 31, 2022 11:48
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Jan 9, 2023
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request May 10, 2023
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request May 10, 2023
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Jun 17, 2023
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Jun 17, 2023
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Aug 2, 2023
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
Stephan202 added a commit to PicnicSupermarket/error-prone that referenced this pull request Aug 5, 2023
The `BaseErrorProneJavaCompiler` already supported this, but
`ErrorProneOptions` prevented the feature from being used. Concrete changes:

1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not
   vice versa.
2. Omit empty strings from `-XepPatchChecks:`.

See google#947.
@cushon
Copy link
Collaborator

cushon commented Sep 11, 2023

(@Stephan202 thanks for the PR, and for reminding me about this one in #4028)

copybara-service bot pushed a commit that referenced this pull request Sep 13, 2023
The `BaseErrorProneJavaCompiler` [supports](https://github.com/google/error-prone/blob/master/check_api/src/main/java/com/google/errorprone/BaseErrorProneJavaCompiler.java#L225) running _all_ patch checks; it just requires that one doesn't provide any explicitly named checkers.

In practice this doesn't work, however, because:
- `PatchingOptions` [requires](https://github.com/google/error-prone/blob/master/check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java#L142) a named checker or a custom refactorer.
- Specifying `-XepPatchChecks:` without any explicit check will [cause](https://github.com/google/error-prone/blob/master/check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java#L404) the empty string to be stored as a named checker.

This PR proposes to resolve the above issues by:
1. Requiring `-XepPatchLocation` when `-XepPatchChecks` is specified, but not vice versa.
2. Omitting empty strings from `-XepPatchChecks:`.

Fixes #947

FUTURE_COPYBARA_INTEGRATE_REVIEW=#947 from PicnicSupermarket:bugfix/support-empty-PatchChecks c5d9125
PiperOrigin-RevId: 564467970
@Stephan202 Stephan202 deleted the bugfix/support-empty-PatchChecks branch September 14, 2023 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants