[Variant] Add human-readable impl Debug for Variant#8140
Conversation
| "decimal16": Decimal16( | ||
| VariantDecimal16 { | ||
| integer: 123456789012345678901234567890, | ||
| scale: 4, | ||
| }, |
There was a problem hiding this comment.
It was very tempting to do something more clever for these decimal subtypes, but ultimately I decided that this is Debug which generally matches the physical layout of the objects even when it's a bit verbose.
Ditto for ShortString.
Happy to reconsider tho!
| // helper to print <invalid> instead of "<invalid>" in debug mode when a VariantObject or VariantList contains invalid values. | ||
| struct InvalidVariant; | ||
|
|
||
| impl std::fmt::Debug for InvalidVariant { |
There was a problem hiding this comment.
Good idea? Bad? Terrible?
There was a problem hiding this comment.
I think it would be clearer if you just used the the string directly.
So instead of
Err(_) => map.entry(&InvalidVariant, &InvalidVariant),do
Err(_) => map.entry(&"invalid", &"invalid"),There was a problem hiding this comment.
That's what I had at first, but it would print e.g.
{
"invalid": "invalid",
}
Since "invalid": "invalid" is valid JSON, it seemed better to have something unambiguous:
{
<invalid>: <invalid>,
}
There was a problem hiding this comment.
But there's certainly an argument to be made that this is overkill for an error case?
There was a problem hiding this comment.
I think it is fine. Let's go with this and if someone else comes up with something better we can use that
| // helper to print binary data in hex format in debug mode, as space-separated hex byte values. | ||
| struct HexString<'a>(&'a [u8]); | ||
|
|
||
| impl<'a> std::fmt::Debug for HexString<'a> { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| if let Some((first, rest)) = self.0.split_first() { |
There was a problem hiding this comment.
Most hex editors use this format, 01 02 03 04 de ad be ef as more compact and readable than something comma-separated.
I intentionally chose not to honor alt syntax for this one because IMO both of these look weird:
Binary(
),
Binary(
01 02 03 04 de ad be ef,
),
vs.
Binary(),
Binary(01 02 03 04 de ad be ef),
Happy to consider other perspectives tho!
(honestly, I wish the alt-printing for tuple types would always avoid line breaks when there's only a single value, but oh well)
| // helper to print <invalid> instead of "<invalid>" in debug mode when a VariantObject or VariantList contains invalid values. | ||
| struct InvalidVariant; | ||
|
|
||
| impl std::fmt::Debug for InvalidVariant { |
There was a problem hiding this comment.
I think it would be clearer if you just used the the string directly.
So instead of
Err(_) => map.entry(&InvalidVariant, &InvalidVariant),do
Err(_) => map.entry(&"invalid", &"invalid"),| // helper to print binary data in hex format in debug mode, as space-separated hex byte values. | ||
| struct HexString<'a>(&'a [u8]); | ||
|
|
||
| impl<'a> std::fmt::Debug for HexString<'a> { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| if let Some((first, rest)) = self.0.split_first() { |
| "decimal16": Decimal16( | ||
| VariantDecimal16 { | ||
| integer: 123456789012345678901234567890, | ||
| scale: 4, | ||
| }, |
|
Thank you @scovich |
2 participants
Which issue does this PR close?
Rationale for this change
Unit tests need a way to verify two
Variantare logically equivalent, andDebugis a good way to achieve that without wading into the complexities of a properPartialEqimplementation that would become part of the public API.More generally, byte slices are not very easy for humans to interpret, so it makes sense for
Debugto do something nicer.What changes are included in this PR?
Manually
impl Debug for Variant, maintaining a traditional look but with nicer handling ofVariant::Binary,Variant::ObjectandVariant::Listsubtypes. The latter two use fallible iteration to avoid potential panics, sinceDebugis likely to be used when formatting error messages.Are these changes tested?
New unit test
Are there any user-facing changes?
The debug formatting of
Varianthas changed.