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

feat(textual): Value Renderers for numbers #12088

Merged
merged 34 commits into from
Jul 25, 2022
Merged

feat(textual): Value Renderers for numbers #12088

merged 34 commits into from
Jul 25, 2022

Conversation

amaury1093
Copy link
Contributor

@amaury1093 amaury1093 commented May 30, 2022

Description

closes #12613

This PR sets up the infrastructure for value renderers as discussed in ADR-050. It's only additive, in a new package, and isn't wired up to the rest of the codebase.

It adds a new root go module tx, which has one textual subpackage for now.

Only adds implementation for:

  • integers
  • decimals

Sister PR:


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@amaury1093 amaury1093 changed the title feat(textual): Value Rendered for numbers and coins feat(textual): Value Renderers for numbers and coins May 30, 2022
@amaury1093 amaury1093 mentioned this pull request May 30, 2022
39 tasks
@amaury1093 amaury1093 changed the title feat(textual): Value Renderers for numbers and coins feat(textual): Value Renderers May 30, 2022
@@ -0,0 +1,177 @@
package textual
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aaronc With the new app wiring, where should this package go? Will we have a root tx/ go mod?

@amaury1093 amaury1093 changed the title feat(textual): Value Renderers feat(textual): Value Renderers for numbers and coins Jun 3, 2022
@amaury1093 amaury1093 changed the title feat(textual): Value Renderers for numbers and coins feat(textual): Value Renderers for numbers Jul 18, 2022
@amaury1093 amaury1093 marked this pull request as ready for review July 18, 2022 13:59
@amaury1093 amaury1093 requested a review from a team as a code owner July 18, 2022 13:59
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

Generally looks like the right direction. Let's switch to a more efficient Reader/Writer implementation. We may want to pre-calculate buffer sizes like the orm does

// here, so that optionally more value renderers could be built, for example, a
// separate one for a different language.
type ValueRenderer interface {
Format(context.Context, protoreflect.Value) ([]string, error)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Format(context.Context, protoreflect.Value) ([]string, error)
Format(context.Context, protoreflect.Value, io.Writer) error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess the downside is that value renderers would need to deal with \n chars all over. By keeping []string we can just do a single .Join() at the very end.

Copy link
Member

Choose a reason for hiding this comment

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

Do they need to deal with new lines actually? Shouldn't that be dealt with higher up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For when the FieldDescriptor is a message field, there'll be new lines (in a future PR).

@amaury1093
Copy link
Contributor Author

OK I changed to io.Reader/Writer in dbe153b. I still think []string improves code readability (wrt to dealing with \n), but we can change it easily if needed.

@aaronc
Copy link
Member

aaronc commented Jul 19, 2022

Not seeing where there's a new line issue

@amaury1093
Copy link
Contributor Author

OK, this is R4R again, and uses the new math.LegacyDec

@amaury1093
Copy link
Contributor Author

OK I created a new tx module, and addressed the last nits. Would love to have a final review and merge.

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Jul 25, 2022
@codecov
Copy link

codecov bot commented Jul 25, 2022

Codecov Report

Merging #12088 (0857782) into main (ef8049a) will increase coverage by 0.02%.
The diff coverage is 62.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #12088      +/-   ##
==========================================
+ Coverage   65.32%   65.34%   +0.02%     
==========================================
  Files         689      692       +3     
  Lines       70706    70788      +82     
==========================================
+ Hits        46191    46259      +68     
- Misses      21960    21966       +6     
- Partials     2555     2563       +8     
Impacted Files Coverage Δ
tx/textual/valuerenderer/dec.go 46.15% <46.15%> (ø)
tx/textual/valuerenderer/int.go 65.21% <65.21%> (ø)
tx/textual/valuerenderer/valuerenderer.go 72.72% <72.72%> (ø)
x/group/keeper/keeper.go 58.26% <0.00%> (-0.40%) ⬇️
x/distribution/simulation/operations.go 90.42% <0.00%> (+9.57%) ⬆️

@mergify mergify bot merged commit ab3feba into main Jul 25, 2022
@mergify mergify bot deleted the am/textual-number branch July 25, 2022 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Textual Value Renderers for Numbers
4 participants