-
Notifications
You must be signed in to change notification settings - Fork 9
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
Use ArrayListMultimap when values are ColumnValue #5855
Conversation
eda5c6b
to
4bb953f
Compare
4bb953f
to
f3cfcd5
Compare
.map(entry -> entry.getTableName() + "Table") | ||
.collect(Collectors.toList()); | ||
String schemaName = ApiTestSchema.getSchema().getName(); | ||
checkIfFilesAreTheSame(schemaName + "TableFactory"); |
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.
Previously this test was not testing the generated table factory. This explains why tests didn't break in #5843.
I've updated this test to ensure we're testing all generated files.
|
||
private String readFileIntoString(File baseDir, String path) throws IOException { | ||
return new String(Files.toByteArray(new File(baseDir, path)), StandardCharsets.UTF_8); | ||
assertThat(expectedFile).hasSameTextualContentAs(actualFile); |
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.
Simplified tests to use AssertJ's AbstractFileAssert
instead of our hand-rolled line-based comparison.
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.
nice, thanks! yeah that makes sense :)
@@ -110,6 +110,7 @@ public TableClassRendererV2(String packageName, Namespace namespace, String rawT | |||
|
|||
public String render() { | |||
JavaFile javaFile = JavaFile.builder(packageName, this.buildTypeSpec()) | |||
.skipJavaLangImports(true) |
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.
This matches what Conjure does:
https://github.com/palantir/conjure-java/search?q=skipJavaLangImports
return Renderers.CamelCase(index.getIndexName()); | ||
} else { | ||
public static String getIndexTableName(IndexMetadata index) { | ||
if (index.getJavaIndexName() != null) { |
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.
Small refactor for consistency with getClassTableName
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.
👍 this looks good. Thanks for the contribution!
|
||
private String readFileIntoString(File baseDir, String path) throws IOException { | ||
return new String(Files.toByteArray(new File(baseDir, path)), StandardCharsets.UTF_8); | ||
assertThat(expectedFile).hasSameTextualContentAs(actualFile); |
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.
nice, thanks! yeah that makes sense :)
ColumnValue
values do not overrideequals
and are compared using reference equality. When creating multimaps withColumnValue
values we can just useArrayListMultimap
to avoid the comparison overhead (and we shouldn't be using reference equality anyways).This matches the behavior of
getAffectedCells
, which also uses anArrayListMultimap
to store a multimap withColumnValue
values.