-
Notifications
You must be signed in to change notification settings - Fork 39
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,114 @@ | ||
package tech.picnic.errorprone.refasterrules; | ||
|
||
import static com.google.common.collect.ImmutableTable.toImmutableTable; | ||
import static com.google.errorprone.refaster.ImportPolicy.STATIC_IMPORT_ALWAYS; | ||
|
||
import com.google.common.collect.ImmutableTable; | ||
import com.google.common.collect.Table; | ||
import com.google.common.collect.Tables; | ||
import com.google.errorprone.refaster.Refaster; | ||
import com.google.errorprone.refaster.annotation.AfterTemplate; | ||
import com.google.errorprone.refaster.annotation.BeforeTemplate; | ||
import com.google.errorprone.refaster.annotation.MayOptionallyUse; | ||
import com.google.errorprone.refaster.annotation.Placeholder; | ||
import com.google.errorprone.refaster.annotation.UseImportPolicy; | ||
import java.util.stream.Stream; | ||
import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; | ||
|
||
/** Refaster rules related to expressions dealing with {@link ImmutableTable}s. */ | ||
@OnlineDocumentation | ||
final class ImmutableTableRules { | ||
private ImmutableTableRules() {} | ||
|
||
/** Prefer {@link ImmutableTable#builder()} over the associated constructor. */ | ||
// XXX: This rule may drop generic type information, leading to non-compilable code. | ||
static final class ImmutableTableBuilder<R, C, V> { | ||
@BeforeTemplate | ||
ImmutableTable.Builder<R, C, V> before() { | ||
return new ImmutableTable.Builder<>(); | ||
} | ||
|
||
@AfterTemplate | ||
ImmutableTable.Builder<R, C, V> after() { | ||
return ImmutableTable.builder(); | ||
} | ||
} | ||
|
||
/** | ||
* Prefer {@link ImmutableTable.Builder#buildOrThrow()} over the less explicit {@link | ||
* ImmutableTable.Builder#build()}. | ||
*/ | ||
static final class ImmutableTableBuilderBuildOrThrow<R, C, V> { | ||
@BeforeTemplate | ||
ImmutableTable<R, C, V> before(ImmutableTable.Builder<R, C, V> builder) { | ||
return builder.build(); | ||
} | ||
|
||
@AfterTemplate | ||
ImmutableTable<R, C, V> after(ImmutableTable.Builder<R, C, V> builder) { | ||
return builder.buildOrThrow(); | ||
} | ||
} | ||
|
||
/** Prefer {@link ImmutableTable#of(Object, Object, Object)} over more contrived alternatives. */ | ||
static final class CellToImmutableTable<R, C, V> { | ||
@BeforeTemplate | ||
ImmutableTable<R, C, V> before(Table.Cell<? extends R, ? extends C, ? extends V> cell) { | ||
return Refaster.anyOf( | ||
ImmutableTable.<R, C, V>builder().put(cell).buildOrThrow(), | ||
Stream.of(cell) | ||
.collect( | ||
toImmutableTable( | ||
Table.Cell::getRowKey, Table.Cell::getColumnKey, Table.Cell::getValue))); | ||
} | ||
|
||
@AfterTemplate | ||
ImmutableTable<R, C, V> after(Table.Cell<? extends R, ? extends C, ? extends V> cell) { | ||
return ImmutableTable.of(cell.getRowKey(), cell.getColumnKey(), cell.getValue()); | ||
} | ||
} | ||
|
||
/** | ||
* Don't map a stream's elements to table cells, only to subsequently collect them into an {@link | ||
* ImmutableTable}. The collection can be performed directly. | ||
*/ | ||
abstract static class StreamOfCellsToImmutableTable<E, R, C, V> { | ||
@Placeholder(allowsIdentity = true) | ||
abstract R rowFunction(@MayOptionallyUse E element); | ||
|
||
@Placeholder(allowsIdentity = true) | ||
abstract C columnFunction(@MayOptionallyUse E element); | ||
|
||
@Placeholder(allowsIdentity = true) | ||
abstract V valueFunction(@MayOptionallyUse E element); | ||
|
||
@BeforeTemplate | ||
ImmutableTable<R, C, V> before(Stream<E> stream) { | ||
return stream | ||
.map(e -> Tables.immutableCell(rowFunction(e), columnFunction(e), valueFunction(e))) | ||
.collect( | ||
toImmutableTable( | ||
Table.Cell::getRowKey, Table.Cell::getColumnKey, Table.Cell::getValue)); | ||
} | ||
|
||
@AfterTemplate | ||
@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))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't these can be lambda expressions? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, sorry 🙈 |
||
} | ||
} | ||
|
||
/** Prefer {@link ImmutableTable#of()} over more contrived alternatives . */ | ||
static final class ImmutableTableOf<R, C, V> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wondered the same, but modelled this after |
||
@BeforeTemplate | ||
ImmutableTable<R, C, V> before() { | ||
return ImmutableTable.<R, C, V>builder().buildOrThrow(); | ||
} | ||
|
||
@AfterTemplate | ||
ImmutableTable<R, C, V> after() { | ||
return ImmutableTable.of(); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
package tech.picnic.errorprone.refasterrules; | ||
|
||
import static com.google.common.collect.ImmutableTable.toImmutableTable; | ||
|
||
import com.google.common.collect.ImmutableSet; | ||
import com.google.common.collect.ImmutableTable; | ||
import com.google.common.collect.Table; | ||
import com.google.common.collect.Tables; | ||
import java.util.stream.Stream; | ||
import tech.picnic.errorprone.refaster.test.RefasterRuleCollectionTestCase; | ||
|
||
final class ImmutableTableRulesTest implements RefasterRuleCollectionTestCase { | ||
@Override | ||
public ImmutableSet<Object> elidedTypesAndStaticImports() { | ||
return ImmutableSet.of(Table.class); | ||
} | ||
|
||
ImmutableTable.Builder<String, Integer, String> testImmutableTableBuilder() { | ||
return new ImmutableTable.Builder<>(); | ||
} | ||
|
||
ImmutableTable<Object, Object, Object> testImmutableTableBuilderBuildOrThrow() { | ||
return ImmutableTable.builder().build(); | ||
} | ||
|
||
ImmutableSet<ImmutableTable<String, Integer, String>> testCellToImmutableTable() { | ||
return ImmutableSet.of( | ||
ImmutableTable.<String, Integer, String>builder() | ||
.put(Tables.immutableCell("foo", 1, "bar")) | ||
.buildOrThrow(), | ||
Stream.of(Tables.immutableCell("baz", 2, "qux")) | ||
.collect( | ||
toImmutableTable( | ||
Table.Cell::getRowKey, Table.Cell::getColumnKey, Table.Cell::getValue))); | ||
} | ||
|
||
ImmutableTable<Integer, String, Integer> testStreamOfCellsToImmutableTable() { | ||
return Stream.of(1, 2, 3) | ||
.map(n -> Tables.immutableCell(n, n.toString(), n * 2)) | ||
.collect( | ||
toImmutableTable( | ||
Table.Cell::getRowKey, Table.Cell::getColumnKey, Table.Cell::getValue)); | ||
} | ||
|
||
ImmutableTable<String, String, String> testImmutableTableOf() { | ||
return ImmutableTable.<String, String, String>builder().buildOrThrow(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
package tech.picnic.errorprone.refasterrules; | ||
|
||
import static com.google.common.collect.ImmutableTable.toImmutableTable; | ||
|
||
import com.google.common.collect.ImmutableSet; | ||
import com.google.common.collect.ImmutableTable; | ||
import com.google.common.collect.Table; | ||
import com.google.common.collect.Tables; | ||
import java.util.stream.Stream; | ||
import tech.picnic.errorprone.refaster.test.RefasterRuleCollectionTestCase; | ||
|
||
final class ImmutableTableRulesTest implements RefasterRuleCollectionTestCase { | ||
@Override | ||
public ImmutableSet<Object> elidedTypesAndStaticImports() { | ||
return ImmutableSet.of(Table.class); | ||
} | ||
|
||
ImmutableTable.Builder<String, Integer, String> testImmutableTableBuilder() { | ||
return ImmutableTable.builder(); | ||
} | ||
|
||
ImmutableTable<Object, Object, Object> testImmutableTableBuilderBuildOrThrow() { | ||
return ImmutableTable.builder().buildOrThrow(); | ||
} | ||
|
||
ImmutableSet<ImmutableTable<String, Integer, String>> testCellToImmutableTable() { | ||
return ImmutableSet.of( | ||
ImmutableTable.of( | ||
Tables.immutableCell("foo", 1, "bar").getRowKey(), | ||
Tables.immutableCell("foo", 1, "bar").getColumnKey(), | ||
Tables.immutableCell("foo", 1, "bar").getValue()), | ||
ImmutableTable.of( | ||
Tables.immutableCell("baz", 2, "qux").getRowKey(), | ||
Tables.immutableCell("baz", 2, "qux").getColumnKey(), | ||
Tables.immutableCell("baz", 2, "qux").getValue())); | ||
} | ||
|
||
ImmutableTable<Integer, String, Integer> testStreamOfCellsToImmutableTable() { | ||
return Stream.of(1, 2, 3).collect(toImmutableTable(n -> n, n -> n.toString(), n -> n * 2)); | ||
} | ||
|
||
ImmutableTable<String, String, String> testImmutableTableOf() { | ||
return ImmutableTable.of(); | ||
} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. :)