rdblue commented on code in PR #16867:
URL: https://github.com/apache/iceberg/pull/16867#discussion_r3502404145
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java:
##########
@@ -392,6 +546,30 @@ private static TrackedFileStruct
dummyTrackedFile(FileContent contentType) {
return file;
}
+ // Builder for a manifest entry with the required non-tracking fields set.
Callers add the
+ // manifest info and any optional fields before building.
+ private static TrackedFileBuilder manifestBuilder(FileContent contentType) {
Review Comment:
I don't think that the complexity here to use the builder is a good thing.
This creates indirection and makes tests harder to read, while gaining
almost nothing. There is hardly any readability benefit to using the builder
when you're already setting constants like `FORMAT_VERSION_V4`. All this does
is move the initialization here, so people have to find this method to validate
that the assertions above are correct, like
`assertThat(manifest.path()).isEqualTo(MANIFEST_FILE_LOCATION)`. I would rather
see `MANIFEST_FILE_LOCATION` passed in and then validated.
I think this should be removed and the constructor should be used instead.
All these tests need to verify is that these values are passed through
correctly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
