Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Textual Any Renderer: Remove msg header screen #14170

Closed
Tracked by #11970
amaury1093 opened this issue Dec 6, 2022 · 2 comments · Fixed by #15208
Closed
Tracked by #11970

Textual Any Renderer: Remove msg header screen #14170

amaury1093 opened this issue Dec 6, 2022 · 2 comments · Fixed by #15208
Assignees

Comments

@amaury1093
Copy link
Contributor

amaury1093 commented Dec 6, 2022

Summary

The proposal is to remove the line in red in the current Any value renderer.

{ "text": "This transaction has 1 Message" },
{ "text": "Message (1/1): Object: /cosmos.bank.v1beta1.MsgSend" },
{ "text": "Message (1/1): /cosmos.bank.v1beta1.MsgSend" },
- { "text": "MsgSend object" },
{ "text": "From address: cosmos1ulav3hsenupswqfkw2y3sup5kgtqwnvqa8eyhs" },
{ "text": "To address: cosmos1abc" },
... // indentation omitted

Problem Definition

Context:

Proposal

Pros of removing the line:

  • shorter rendered output (but needs a small "hack" in Any value renderer)

Pros of keeping the line:

  • Consistency, when thinking about value renderers.

For example, for nested fields, we use:

My field: [[MsgSend object
> From address: cosmos1ulav3hsenupswqfkw2y3sup5kgtqwnvqa8eyhs]]

Which has a clear spec: <field name>: <value-rendered msg>, where the part between brackets [[ ]] denotes the message value renderer output (the brackets themselves are not shown on screen, just here to clarify the purpose).

The Any spec is:

<Type URL>
<value-renderer msg>

which gives the initial output with the message header screen.

So it's a tradeoff between shorter screens vs consistency.

@amaury1093
Copy link
Contributor Author

In Jim's PR, I approved to go with the additional header screen, because it felt more correct code-wise.

However, I'm changing my mind, and I think Textual is about human readability before anything, and shorter is better in this case. Morever, I suspect that the above consistency argument only stands because of our chosen implementation.

@JimLarson
Copy link
Contributor

I'm softening on my resistance to this - the extra header of the Any really does make the message header unnecessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants