Skip to content

Ignore disabled checks passed to -XepPatchChecks#62

Closed
oxkitsune wants to merge 2 commits into
masterfrom
gdejong/remove-disabled-checks
Closed

Ignore disabled checks passed to -XepPatchChecks#62
oxkitsune wants to merge 2 commits into
masterfrom
gdejong/remove-disabled-checks

Conversation

@oxkitsune

@oxkitsune oxkitsune commented Jul 17, 2023

Copy link
Copy Markdown

Rather than throwing an NoSuchElementException.

Related issue: google#3908

@rickie rickie left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed offline; we can probably add a test in the ScannerSupplierTest.java which makes it a lot easier to see what we are fixing. Additionally, it's nice to have a reproduction case in the code to quickly iterate.

Added a commit 😄.

Comment thread check_api/src/main/java/com/google/errorprone/scanner/ErrorProneScanner.java Outdated
Comment thread check_api/src/main/java/com/google/errorprone/scanner/ErrorProneScanner.java Outdated
Comment thread check_api/src/main/java/com/google/errorprone/scanner/ErrorProneScanner.java Outdated
Comment thread check_api/src/main/java/com/google/errorprone/scanner/ErrorProneScanner.java Outdated
@oxkitsune oxkitsune force-pushed the gdejong/remove-disabled-checks branch from 163e405 to 9af0e66 Compare July 24, 2023 13:08
@oxkitsune oxkitsune marked this pull request as draft July 26, 2023 07:56
@oxkitsune oxkitsune requested a review from rickie July 26, 2023 08:59
@oxkitsune oxkitsune marked this pull request as ready for review July 26, 2023 08:59

@rickie rickie left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a small commit. Really nice fix @oxkitsune 🚀 !

Suggested commit message:

Don't consider disabled checks passed to `-XepPatchChecks`

Fixes #3908.

Alternative suggested commit message:

Have `ScannerSupplier#applyOverrides` only consider enabled checks

Fixes #3908.

Consider could also be a different word like "modify" or "update".

Not too happy with the suggestions, ideas welcome.

private final CompilationTestHelper compilationHelper =
CompilationTestHelper.newInstance(ShouldNotUseFoo.class, getClass());

@Test

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can move this to the bottom of the class as we are adding it only now. Not a really good reason but it feels like the right thing to do?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't exactly sure if this is the right file for the test, but I agree!

@rickie rickie requested a review from Stephan202 July 27, 2023 15:05
@Stephan202 Stephan202 force-pushed the gdejong/remove-disabled-checks branch from 2dc88b7 to 61cc147 Compare July 30, 2023 10:00

@Stephan202 Stephan202 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased and added a commit. Tweaked suggested commit message:

Ignore disabled checks passed to `-XepPatchChecks`

Rather than throwing an `NoSuchElementException`.

Fixes #3908.

Comment thread core/src/test/java/com/google/errorprone/scanner/ScannerTest.java Outdated
@oxkitsune oxkitsune force-pushed the gdejong/remove-disabled-checks branch from 61cc147 to 4d502e0 Compare July 31, 2023 10:47
@oxkitsune

Copy link
Copy Markdown
Author

Upstream PR: google#4028

@oxkitsune oxkitsune changed the title Prevent Exception from being thrown when a disabled check is passed to -XepPatchChecks Ignore disabled checks passed to -XepPatchChecks Aug 1, 2023
Rather than throwing an `NoSuchElementException`.

Fixes google#3908.
@Stephan202 Stephan202 force-pushed the gdejong/remove-disabled-checks branch from 4d502e0 to 5865373 Compare September 13, 2023 17:27
@Stephan202 Stephan202 closed this Jan 3, 2025
@Stephan202 Stephan202 deleted the gdejong/remove-disabled-checks branch January 3, 2025 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants