Skip to content

Conversation

@adampetro
Copy link
Contributor

No description provided.

#[serde(flatten)]
pub json_value: Option<serde_json::Value>,
/// The human readable representation of the bytes.
#[serde(skip)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I introduced this for the codecs, but given that it's all json, maybe we should remove it while we are at it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use the value here and here when the --json flag isn't passed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but this is mostly to accommodate the raw codec, for which we couldn't make any assumptions when the cli flag was exposed

humanized: "<raw codec>".into(),
e.g., we couldn't assume that it could be represented as json, for all the other cases, humanized is pretty much a json representation if I'm not wrong? So we could drop humanized and simply stringify the json directly. OTOH, maybe it's fine to keep it, and instead we should remove the Raw option from the codec which is what really complicates things here. I'll leave it up to you in this case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should probably remove the Raw codec. I'm fine doing that outside of this PR though.

@saulecabrera saulecabrera marked this pull request as ready for review May 16, 2025 19:11
#[serde(flatten)]
pub json_value: Option<serde_json::Value>,
/// The human readable representation of the bytes.
#[serde(skip)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should probably remove the Raw codec. I'm fine doing that outside of this PR though.

@andrewhassan andrewhassan merged commit f83095b into main May 16, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants