Adopt YAML as human-readable text output #4433

alessio · 2019-05-29T10:24:42Z

Closes: #4371

Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added a relevant changelog entry: clog add [section] [stanza] [message]
rereviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

Closes: #4371

codecov · 2019-05-29T10:50:03Z

Codecov Report

Merging #4433 into master will decrease coverage by 0.08%.
The diff coverage is 25.53%.

@@            Coverage Diff            @@
##           master   #4433      +/-   ##
=========================================
- Coverage   54.58%   54.5%   -0.09%     
=========================================
  Files         248     248              
  Lines       15967   15987      +20     
=========================================
- Hits         8716    8714       -2     
- Misses       6617    6636      +19     
- Partials      634     637       +3

alexanderbez

Change LGTM, but I fear we may have to update a lot of the structs that are returned as responses.

some examples:

$ gaiacli q staking validators
- operatoraddress:
  - 145
  - 123
  - 100
  - 232
  - 248
  - 88
  - 243
  - 130
  - 213
  - 110
  - 29
  - 190
  - 208
  - 108
  - 105
  - 202
  - 24
  - 133
  - 105
  - 95
  conspubkey:
  - 6
  - 117
  - 186
  - 8
  - 211
  - 175
  - 221
  - 74
  - 229
  - 35
  - 72
  - 126
  - 75
  - 16
  - 12
  - 70
  - 37
  - 220
  - 88
  - 187
  - 173
  - 1
  - 31
  - 96
  - 18
  - 35
  - 71
  - 16
  - 246
  - 140
  - 84
  - 239
  jailed: false
  status: 2
  tokens: {}
  delegatorshares: "100000000000000000000000000"
  description:
    moniker: node0
    identity: ""
    website: ""
    details: ""
  unbondingheight: 0
  unbondingcompletiontime: 1970-01-01T00:00:00Z
  commission:
    rate: "0"
    maxrate: "0"
    maxchangerate: "0"
    updatetime: 2019-05-29T13:18:00.214626Z
  minselfdelegation: {}

fedekunze · 2019-05-29T13:35:00Z

why doesn't it use snakecase ? eg unbondingcompletiontime should say unbonding_completion_time

sabau · 2019-05-29T14:11:42Z

I was playing around this, can be https://godoc.org/gopkg.in/yaml.v2#Marshal the cause of this weird naming? seems the correct name have to be specified.

I think what @alexanderbez said will help with the naming too. Should we specify their yaml counterpart as we do with json?

alessio · 2019-05-29T14:53:35Z

$ gaiacli q staking validators

operatoraddress:

145

123

100

232

248

88

243

130

213

110

29

190

208

108

105

202

24

133

105

95

Wow, quite ugly indeed - let's see what I can do for it

alessio · 2019-05-30T03:00:14Z

@alexanderbez please test again - I think addresses now look alright

sabau · 2019-05-30T13:03:06Z

Seems healthier now.
An optional (time consuming but easy) following PR may fix the naming

sabau

ACK.
If we postpone the copy-pastable names to fix them later seems ok now

alexanderbez · 2019-05-30T13:55:49Z

I'm not sure what I'm doing wrong, but I still see this (using the latest commit 82ce39e7e5b6 on gaia):

- operatoraddress:
  - 225
  - 216
  - 202
  - 221
  - 78
  - 93
  - 125
  - 14
  - 121
  - 203
  - 85
  - 100
  - 151
  - 241
  - 191
  - 63
  - 37
  - 8
  - 61
  - 208
  conspubkey:
  - 58
  - 150
  - 204
  - 224
  - 117
  - 51
  - 114
  - 38
  - 46
  - 182
  - 222
  - 88
  - 44
  - 23
  - 56
  - 140
  - 43
  - 138
  - 196
  - 226
  - 183
  - 116
  - 121
  - 167
  - 224
  - 109
  - 182
  - 22
  - 206
  - 107
  - 255
  - 13
  jailed: false
  status: 2
  tokens: {}
  delegatorshares: "100000000000000000000000000"
  description:
    moniker: node3
    identity: ""
    website: ""
    details: ""
  unbondingheight: 0
  unbondingcompletiontime: 1970-01-01T00:00:00Z
  commission:
    rate: "0"
    maxrate: "0"
    maxchangerate: "0"
    updatetime: 2019-05-30T13:44:07.3021038Z
  minselfdelegation: {}

alexanderbez · 2019-05-30T17:58:01Z

@alessio this definitely resolved the addresses -- thanks! My concern is with types that are defined outside the SDK (eg. crypto.PubKey).

How do you suggest we go about these types? Only thing that comes to mind is alias types or embedding. Kind of annoying, but I think manageable.

rigelrozanski

couple comments

client/context/context.go

rigelrozanski · 2019-05-30T22:36:48Z

client/keys/show.go

@@ -113,8 +110,6 @@ func runShowCmd(cmd *cobra.Command, args []string) (err error) {
 		printKeyAddress(info, bechKeyOut)
 	case isShowPubKey:
 		printPubKey(info, bechKeyOut)
-	case isShowMultiSig:
-		printMultiSigKeyInfo(info, bechKeyOut)


Why are removing this btw?

We don't need an extra flag anymore to display a tree of multisig keys

e.g. this comes out of the box

- name: multi type: multi address: cosmos195eza9hyjx9w4amt3he37hqdcv66tjx8uwpapw pubkey: cosmospub1ytql0csgqyfzd666axrjzq3zqh4r75vtgxx2cndqpgy60yc9tfu9xxgwmqqnshgkdw79wnuv8ufzd666axrjzq39tr0mg9xyvelgmq4qxvm9ytvf2flduzwfmhd62fkvpjunfmzg0u8h900d mnemonic: "" threshold: 1 pubkeys: - address: cosmos1dj4ghqevu9y788gkc6gafdpgx6umh8qplffye4 pubkey: cosmospub1addwnpepqg3qt63l2x95rr9vfksq5zd8jvz457znry8dsqfct5txh0zhf7xr7yssng9 weight: 1 - address: cosmos104ytdpvrx9284zd50v9ep8c6j7pua7dkk0x3ek pubkey: cosmospub1addwnpepqgj43ha5znzxvl5ds2srxdjj9ky4ylk7p8yamka9ymxqewf5a3y870kwj76 weight: 1

alessio · 2019-05-31T08:18:05Z

@alexanderbez I haven't yet stumbled across funnily serialized types, e.g. PubKey serialization seems to work. Concerning upstream types, we generally embed them into our structures/types, I think the cheapest and quicketst solution would be writing MarshalYAML() methods on case-by-case basis

alexanderbez

TestedACK. We'll need to do some spring cleaning and add YAML tags to all the necessary types. I'm sure this can easily be done via tool.

Adopt YAML as human-readable text output

765e0f0

Closes: #4371

alessio requested review from sabau and fedekunze May 29, 2019 10:24

alessio requested review from alexanderbez, jackzampolin and rigelrozanski as code owners May 29, 2019 10:24

alexanderbez added ready-for-review T: UX labels May 29, 2019

alexanderbez suggested changes May 29, 2019

View reviewed changes

Implement YAML Marshalling for addresses

6116ca1

Support YAML unmarshalling (mostly for testing purposes)

82ce39e

alessio requested a review from alexanderbez May 30, 2019 03:10

sabau approved these changes May 30, 2019

View reviewed changes

alessio added 2 commits May 30, 2019 15:18

Fix YAML marshalling

6933233

Fix YAML marshaling

f7a93e2

rigelrozanski approved these changes May 30, 2019

View reviewed changes

Address @rigelrozanski comment - remove unnecessary import alias

6fcc5f9

alexanderbez approved these changes May 31, 2019

View reviewed changes

alexanderbez merged commit e9810ac into master May 31, 2019

alexanderbez deleted the alessio/yaml-as-text-output branch May 31, 2019 13:14

fedekunze mentioned this pull request Jun 24, 2019

YAML output for CLI #4612

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopt YAML as human-readable text output #4433

Adopt YAML as human-readable text output #4433

alessio commented May 29, 2019

codecov bot commented May 29, 2019 •

edited

Loading

alexanderbez left a comment

fedekunze commented May 29, 2019

sabau commented May 29, 2019 •

edited

Loading

alessio commented May 29, 2019

alessio commented May 30, 2019

sabau commented May 30, 2019

sabau left a comment

alexanderbez commented May 30, 2019 •

edited

Loading

alexanderbez commented May 30, 2019

rigelrozanski left a comment

rigelrozanski May 30, 2019

alessio May 31, 2019 •

edited

Loading

alessio May 31, 2019

alessio commented May 31, 2019

alexanderbez left a comment

Adopt YAML as human-readable text output #4433

Adopt YAML as human-readable text output #4433

Conversation

alessio commented May 29, 2019

codecov bot commented May 29, 2019 • edited Loading

Codecov Report

alexanderbez left a comment

Choose a reason for hiding this comment

fedekunze commented May 29, 2019

sabau commented May 29, 2019 • edited Loading

alessio commented May 29, 2019

alessio commented May 30, 2019

sabau commented May 30, 2019

sabau left a comment

Choose a reason for hiding this comment

alexanderbez commented May 30, 2019 • edited Loading

alexanderbez commented May 30, 2019

rigelrozanski left a comment

Choose a reason for hiding this comment

rigelrozanski May 30, 2019

Choose a reason for hiding this comment

alessio May 31, 2019 • edited Loading

Choose a reason for hiding this comment

alessio May 31, 2019

Choose a reason for hiding this comment

alessio commented May 31, 2019

alexanderbez left a comment

Choose a reason for hiding this comment

codecov bot commented May 29, 2019 •

edited

Loading

sabau commented May 29, 2019 •

edited

Loading

alexanderbez commented May 30, 2019 •

edited

Loading

alessio May 31, 2019 •

edited

Loading