RussellSpitzer commented on code in PR #11948:
URL: https://github.com/apache/iceberg/pull/11948#discussion_r1916925759
##########
core/src/test/java/org/apache/iceberg/TestSnapshotJson.java:
##########
@@ -43,7 +43,16 @@ public void testJsonConversion() throws IOException {
Snapshot expected =
new BaseSnapshot(
Review Comment:
I'm strongly against multiple constructors because I think the use cases we
have here (mostly test cases) don't actually need to specify all of the
parameters. Currently we have tests like
```java
testParameter5 () {
Snapshot = (foo, bar, x ,null, 4, ...,....,.,,..)
}
```
Which I think would be much cleaner as
```java
testParameter5() {
testSnapshot().withParameter5(true) ... some beahvior
testSnapshot().withParameter5(false) .. some other behavior
}
```
Not only would the new tests be cleaner, but they would also be future
proofed.
For the general use in the library I don't think there are any places in the
code where we actually want to be using a constructor with less parameters. For
Snapshot, we always want everything specified and for TableMetadata we expect
users to go through the Public Builder and not the constructor.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
