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 ImmutableTableRules Refaster rule collection #1489

Merged
merged 2 commits into from
Jan 3, 2025

Conversation

Stephan202
Copy link
Member

@Stephan202 Stephan202 commented Dec 29, 2024

Suggested commit message:

Introduce `ImmutableTableRules` Refaster rule collection (#1489)

This work results from #1223 and this comment, with the observation that ImmutableTable has a distinct builder type. The rules introduced here are a subset of those defined by ImmutableMapRules.

Copy link

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.

Copy link

Looks good. All 1 mutations in this change were killed.

class surviving killed
🎉tech.picnic.errorprone.bugpatterns.NonStaticImport 0 1

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Copy link
Contributor

@mohamedsamehsalah mohamedsamehsalah left a comment

Choose a reason for hiding this comment

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

🪓

Copy link
Contributor

@mohamedsamehsalah mohamedsamehsalah left a comment

Choose a reason for hiding this comment

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

Sorry, I wanted to add some comments; leaving approval otherwise 👍

@UseImportPolicy(STATIC_IMPORT_ALWAYS)
ImmutableTable<R, C, V> after(Stream<E> stream) {
return stream.collect(
toImmutableTable(e -> rowFunction(e), e -> columnFunction(e), e -> valueFunction(e)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't these can be lambda expressions?

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean method references? 🙃

Unfortunately, Refaster doesn't have a "make this a method reference if possible" feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean method references? 🙃

Yes, sorry 🙈

private ImmutableTableRules() {}

/** Prefer {@link ImmutableTable#builder()} over the associated constructor. */
// XXX: This rule may drop generic type information, leading to non-compilable code.
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically it does not drop all generic type information, correct?

Suggested change
// XXX: This rule may drop generic type information, leading to non-compilable code.
// XXX: This rule may drop explicit generic type information, leading to non-compilable code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, but it also says "may", so it's clear that anyway this is an issue only in a subset of cases. :)

Copy link
Member Author

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

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

Tnx for the review @mohamedsamehsalah!

private ImmutableTableRules() {}

/** Prefer {@link ImmutableTable#builder()} over the associated constructor. */
// XXX: This rule may drop generic type information, leading to non-compilable code.
Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, but it also says "may", so it's clear that anyway this is an issue only in a subset of cases. :)

@UseImportPolicy(STATIC_IMPORT_ALWAYS)
ImmutableTable<R, C, V> after(Stream<E> stream) {
return stream.collect(
toImmutableTable(e -> rowFunction(e), e -> columnFunction(e), e -> valueFunction(e)));
Copy link
Member Author

Choose a reason for hiding this comment

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

You mean method references? 🙃

Unfortunately, Refaster doesn't have a "make this a method reference if possible" feature.

@Stephan202
Copy link
Member Author

Validated that these changes do not impact the integration tests.

}

/** Prefer {@link ImmutableTable#of()} over more contrived alternatives . */
static final class ImmutableTableOf<R, C, V> {
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to move this Refaster rule a bit more up? Like right after the builder or the ImmutableTableBuilderBuildOrThrow one?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wondered the same, but modelled this after ImmutableMapRules 🤷. (If/when we decide to auto-sort rules, this kind of more semantic approach to ordering will likely go out the window. Something to consider.)

@rickie rickie force-pushed the sschroevers/additional-immutable-table-rules branch from 3e9746d to 7d2a94d Compare January 3, 2025 07:51
Copy link

github-actions bot commented Jan 3, 2025

Looks good. All 1 mutations in this change were killed.

class surviving killed
🎉tech.picnic.errorprone.bugpatterns.NonStaticImport 0 1

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@Stephan202 Stephan202 force-pushed the sschroevers/additional-immutable-table-rules branch from 7d2a94d to 58dcc7d Compare January 3, 2025 08:00
Copy link

github-actions bot commented Jan 3, 2025

Looks good. All 1 mutations in this change were killed.

class surviving killed
🎉tech.picnic.errorprone.bugpatterns.NonStaticImport 0 1

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@rickie rickie merged commit 9493f2d into master Jan 3, 2025
16 checks passed
@rickie rickie deleted the sschroevers/additional-immutable-table-rules branch January 3, 2025 10:18
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.

3 participants