Variants: Implement toString #12138
Variants: Implement toString #12138
Conversation
| builder.append(", "); | ||
| } | ||
|
|
||
| builder.append(i).append(" => ").append(metadata.get(i)); |
There was a problem hiding this comment.
Nit: Why not just use ":" here as well?
There was a problem hiding this comment.
+1. Basically we are creating an ID => object key string mapping. Seems fine to use ":" to be consistent.
There was a problem hiding this comment.
It's not an object, so this isn't really a key. It's a mapping from ID.
There was a problem hiding this comment.
Sorry. I mean the the strings in the dictionary are the keys from the variant object.
| static String asString(VariantMetadata metadata) { | ||
| StringBuilder builder = new StringBuilder(); |
There was a problem hiding this comment.
Maybe just add as regular method rather than static function.
That also will be consistent with other conversion like as asStructType() in Type.java.
There was a problem hiding this comment.
This can't be added in the interface because it would be a default that conflicts with an existing method (Object#toString). To share the implementations, I added these as static methods in the interfaces and then call them from the concrete classes.
There was a problem hiding this comment.
Yeah. We need to add as default. We are adding asString(), not toString(). Why will that conflict? I tried and seems that works.
| @@ -38,4 +38,23 @@ default Variants.PhysicalType type() { | |||
| default VariantObject asObject() { | |||
| return this; | |||
| } | |||
|
|
|||
| static String asString(VariantObject object) { | |||
There was a problem hiding this comment.
Same here. Probably add as regular class method rather than static funciton.
| } | ||
| } | ||
|
|
||
| static String asString(VariantPrimitive<?> primitive) { |
There was a problem hiding this comment.
Same here. Probably add as regular class method rather than static funciton.
| builder.append(", "); | ||
| } | ||
|
|
||
| builder.append(i).append(" => ").append(metadata.get(i)); |
There was a problem hiding this comment.
+1. Basically we are creating an ID => object key string mapping. Seems fine to use ":" to be consistent.
| @@ -210,4 +210,9 @@ public int writeTo(ByteBuffer outBuffer, int offset) { | |||
|
|
|||
| throw new UnsupportedOperationException("Unsupported primitive type: " + type()); | |||
| } | |||
|
|
|||
| @Override | |||
| public String toString() { | |||
There was a problem hiding this comment.
Can we add basic test coverage for the changes?
There was a problem hiding this comment.
These are for convenience and are not part of a contract. I don't think it is worth adding tests.
There was a problem hiding this comment.
I wish we had better string builders in Java because I would prefer something like
StreamSupport.stream(object.fieldNames().spliterator(), false)
.map(fieldName -> fieldName + ": " + object.get(fieldName) )
.collect(Collectors.joining(","));But that obviously wouldn't use the string builder for the internal strings and has to do the ugly stream support thing. So i'm good with this as is
This implements
toStringfor Variant classes, which is more friendly when reading parameterized test cases.