-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
JSON Codec Updates #6444
JSON Codec Updates #6444
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6444 +/- ##
==========================================
- Coverage 55.79% 55.78% -0.02%
==========================================
Files 465 465
Lines 27800 27809 +9
==========================================
+ Hits 15512 15513 +1
- Misses 11183 11190 +7
- Partials 1105 1106 +1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
@@ -370,10 +370,10 @@ func (ctx Context) PrintOutput(toPrint interface{}) error { | |||
out, err = yaml.Marshal(&toPrint) | |||
|
|||
case "json": | |||
out, err = ctx.JSONMarshaler.MarshalJSON(toPrint) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is now using JSONMarshaler
we don't actually need to deprecate this in favor of Println
do we? Is that right @alexanderbez ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I can't recall why the TODO was added. I think we'll still need this, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main difference is that Println
can use ctx.Output
if it's set instead of stdout, which I think makes sense anyway. I'm doing some fixes to Println
in #6367 and I'm thinking to just update PrintOutput
instead and remove Println
.
Use
cdc.JSONMarshaler
where applicable instead of the deprecatedctx.Codec
.Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorer