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

Introduce ClassRules Refaster rule collection #811

Merged
merged 1 commit into from
Oct 9, 2023

Conversation

Stephan202
Copy link
Member

@Stephan202 Stephan202 commented Oct 4, 2023

Proposal based on this comment.

Suggested commit message:

Introduce `ClassRules` Refaster rule collection (#811)

@Stephan202 Stephan202 added this to the 0.15.0 milestone Oct 4, 2023
@Stephan202 Stephan202 requested review from werli and rickie October 4, 2023 19:46
@github-actions
Copy link

github-actions bot commented Oct 4, 2023

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Comment on lines +14 to +38
/** Prefer {@link Class#isInstance(Object)} over more contrived alternatives. */
static final class ClassIsInstance<T, S> {
@BeforeTemplate
boolean before(Class<T> clazz, S object) {
return clazz.isAssignableFrom(object.getClass());
}

@AfterTemplate
boolean after(Class<T> clazz, S object) {
return clazz.isInstance(object);
}
}

/** Prefer using the {@code instanceof} keyword over less idiomatic alternatives. */
static final class Instanceof<T, S> {
@BeforeTemplate
boolean before(S object) {
return Refaster.<T>clazz().isInstance(object);
}

@AfterTemplate
boolean after(S object) {
return Refaster.<T>isInstance(object);
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

These rules are split out, because the second rewrite is possible only for literal class references.

Comment on lines +40 to +64
/** Prefer {@link Class#isInstance(Object)} method references over more verbose alternatives. */
static final class ClassLiteralIsInstancePredicate<T, S> {
@BeforeTemplate
Predicate<S> before() {
return o -> Refaster.<T>isInstance(o);
}

@AfterTemplate
Predicate<S> after() {
return Refaster.<T>clazz()::isInstance;
}
}

/** Prefer {@link Class#isInstance(Object)} method references over more verbose alternatives. */
static final class ClassReferenceIsInstancePredicate<T, S> {
@BeforeTemplate
Predicate<S> before(Class<T> clazz) {
return o -> clazz.isInstance(o);
}

@AfterTemplate
Predicate<S> after(Class<T> clazz) {
return clazz::isInstance;
}
}
Copy link
Member Author

@Stephan202 Stephan202 Oct 4, 2023

Choose a reason for hiding this comment

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

Hmm, I just realized I don't need to split these out; will add another commit. Similarly here: the first case only applies to literal class references.

Copy link
Member

@werli werli left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Contributor

@Venorcis Venorcis left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻


ImmutableSet<Boolean> testInstanceof() throws IOException {
Class<?> clazz = CharSequence.class;
return ImmutableSet.of(CharSequence.class.isInstance("foo"), clazz.isInstance("bar"));
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, that means we can actually drop this second test case right? What's the reason for keeping it?

Copy link
Member Author

@Stephan202 Stephan202 Oct 6, 2023

Choose a reason for hiding this comment

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

We could, but I wanted to be explicit about covering both cases. We could keep just the second, if you prefer, cause that one is more generic (but that's only clear in the context of the other rules).

Sorry, mixing things up (yes, this PR was harder to write than it looks): that second test case is to prove that non-static references are not replaced, as the result wouldn't compile. (Without this extra check one can replace the Refaster.<T>clazz() with a Class<?> parameter without the test failing; that's what I want to avoid.)

Copy link
Member

@rickie rickie Oct 8, 2023

Choose a reason for hiding this comment

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

Ahh okay clear, let's keep it 😄.

@rickie rickie force-pushed the sschroevers/introduce-class-rules branch from 658fbcb to f0a9de4 Compare October 8, 2023 14:22
@github-actions
Copy link

github-actions bot commented Oct 8, 2023

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@rickie
Copy link
Member

rickie commented Oct 8, 2023

Rebased and tweaked suggested commit message.

@github-actions
Copy link

github-actions bot commented Oct 8, 2023

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@rickie rickie merged commit b1683a3 into master Oct 9, 2023
17 checks passed
@rickie rickie deleted the sschroevers/introduce-class-rules branch October 9, 2023 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants