R4R: CLI UX Updates #1 #1983

mslipper · 2018-08-11T07:05:39Z

This PR closes #1970, #1971, #1967, and #1969.

codecov · 2018-08-11T07:14:55Z

Codecov Report

Merging #1983 into develop will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff            @@
##           develop    #1983   +/-   ##
========================================
  Coverage    63.83%   63.83%           
========================================
  Files          113      113           
  Lines         6684     6684           
========================================
  Hits          4267     4267           
  Misses        2133     2133           
  Partials       284      284

rigelrozanski · 2018-08-11T21:02:40Z

Hey @mslipper ! Thanks for taking the time to make contributions :)
I've noticed you've opened a few PR's recently - as the team is growing quickly and requires fast delegation of reviewing tasks, it would be much appreciated for you to update the titles of your PRs to be more descriptive of the code changes. For instance in this PR what are the UX changes that are taking place? can we summarize those in a few words to include as the title?

thanks again! :)

ValarDragon · 2018-08-11T22:58:13Z

Could you please update the changelog? PENDING.md

ebuchman · 2018-08-12T07:43:07Z

Waiting for after v0.24

mslipper · 2018-08-12T23:50:52Z

@rigelrozanski sure thing!

mslipper · 2018-08-13T23:44:23Z

Pending log updated.

melekes · 2018-08-14T07:22:45Z

PENDING.md

@@ -38,6 +38,8 @@ BREAKING CHANGES
 * [types] sdk.NewCoin now takes sdk.Int, sdk.NewInt64Coin takes int64
 * [cli] #1551: Officially removed `--name` from CLI commands
 * [cli] Genesis/key creation (`init`) now supports user-provided key passwords
+* [cli] --print-response now defaults to true in commands that create and send a transaction
+* [cli] you can now pass --public-key or --address to `gaiacli keys show` to return a plaintext representation of the key's address or public key for use with other commands


I think pubkey is generally accepted abbreviation, so maybe rename to --pubkey?

++

@mslipper may you also reference the PR here?

melekes · 2018-08-14T07:28:48Z

client/flags.go

@@ -19,6 +19,8 @@ const (
 	FlagAsync         = "async"
 	FlagJson          = "json"
 	FlagPrintResponse = "print-response"
+	FlagAddress       = "address"


Would it make sense to move these two const into keys/show.go file?

melekes · 2018-08-14T07:35:36Z

client/keys/show.go

 	},
 }

+func init() {
+	showKeysCmd.Flags().Bool(client.FlagAddress, false, "output the address only (overrides --output)")


Why not have a single --text-format flag, which is set to NAME:\tTYPE:\tADDRESS:\t\t\t\t\t\tPUBKEY by default? Then everybody can choose whatever text format they want.

The objective of this output format is to allow piping to other commands, so we'd need to add additional options to not show that header.

melekes · 2018-08-14T07:36:44Z

client/tx/search.go

+
+For example:
+
+$ gaiacli advanced tendermint txs --tag test1,test2


note advanced will soon be removed #2014

fedekunze · 2018-08-19T21:27:50Z

client/keys/utils.go

 	"github.com/tendermint/tendermint/libs/cli"
 	dbm "github.com/tendermint/tendermint/libs/db"

 	"github.com/cosmos/cosmos-sdk/client"

 	sdk "github.com/cosmos/cosmos-sdk/types"
-)
+	)


remove space

alexanderbez

Thanks for the contrib @mslipper ! I left a bit of feedback + also need to address broken CI 👍

alexanderbez · 2018-08-20T13:50:40Z

client/tx/search.go

+
+For example:
+
+$ gaiacli advanced tendermint txs --tag test1,test2


alexanderbez · 2018-08-20T14:07:56Z

client/keys/show.go

+
+const (
+	FlagAddress       = "address"
+	FlagPublicKey     = "public-key"


alexanderbez · 2018-08-20T14:08:37Z

PENDING.md

@@ -41,6 +41,8 @@ BREAKING CHANGES
 * [cli] Genesis/key creation (`init`) now supports user-provided key passwords
 * [cli] unsafe_reset_all, show_validator, and show_node_id have been renamed to unsafe-reset-all, show-validator, and show-node-id
 * [cli] \#1901 Flag --address-validator renamed to --validator in stake and slashing commands
+* [cli] --print-response now defaults to true in commands that create and send a transaction


Missing PR reference ;-)

alexanderbez · 2018-08-20T14:09:16Z

client/keys/utils.go

+		panic(err)
+	}
+	fmt.Println(ko.PubKey)
+}


Let's add a newline for consistency 👍

alexanderbez

tested ACK -- not too familiar with the CircleCI config/setup, so it might warrant another pair of eyes, but everything else looks fine. Thanks!

jaekwon · 2018-08-21T22:20:48Z

client/keys/show.go

 	"github.com/gorilla/mux"

 	"github.com/spf13/cobra"
+		"github.com/spf13/viper"


gofmt? btw, our (a bit outdated) Go guidelines: https://github.com/tendermint/coding/blob/master/go/coding_standard.md

Our editors are set up so that we always fmt before saving.

jaekwon · 2018-08-21T22:25:09Z

Just a comment but otherwise LGTM

…ptext updates Closes cosmos#1970 Closes cosmos#1971 Closes cosmos#1967 Closes cosmos#1969

jackzampolin · 2018-08-21T23:54:31Z

I think this is ready to go!

cwgoes · 2018-08-22T10:29:41Z

Gopkg.lock

@@ -2,76 +2,57 @@



Upgrade your version of dep, then this shouldn't happen

cwgoes

utACK

* Update dependencies and compile * Add ICS * Add gov prop * Fix lint * Fix commenting * Fix commenting * Add GetStakingKeeper for ICS e2e test * Gaia implements ibc TestingApp * Update correct return type for GetStakingKeeper * Fix ibc import * Fix linting * Comment back LoadStreamingServices * Change IBC back to v3.4 * Fix ics post merge * Replace ibc with informal/ibc for e2e/ibctesting compatibility * Update ibc to 3.4.0 * Fix linting * Update gaia with new ics keeper interface * Add provider module to simulationModules * Bump ics * Fix sdk import * Update sdk to tagged release * Fix simapp dependence (cosmos#1983) * Fix simapp dependence * Fix coverage and linting * Update sim ignore * Update codecov rules * Update ics dependency * Update ICS dependency * Update code cov * Update ICS to tagged version * Update codecov * Add imported ICS test * Make requested test comment changes * Remove commented code Co-authored-by: Milan Mulji <98309852+mmulji-ic@users.noreply.github.com>

mslipper requested review from cwgoes, ebuchman and rigelrozanski as code owners August 11, 2018 07:05

mslipper force-pushed the cli-updates branch from 5595681 to d994c39 Compare August 11, 2018 07:06

mslipper force-pushed the cli-updates branch from d994c39 to e52713f Compare August 13, 2018 23:44

melekes reviewed Aug 14, 2018

View reviewed changes

mslipper force-pushed the cli-updates branch from e52713f to 495c9d4 Compare August 17, 2018 08:29

fedekunze reviewed Aug 19, 2018

View reviewed changes

alexanderbez suggested changes Aug 20, 2018

View reviewed changes

alexanderbez changed the title ~~CLI UX Updates #1~~ WIP: CLI UX Updates #1 Aug 20, 2018

alexanderbez added C:CLI T: UX wip T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). labels Aug 20, 2018

mslipper force-pushed the cli-updates branch 10 times, most recently from 0c410d7 to 7cb3f5b Compare August 21, 2018 19:15

mslipper force-pushed the cli-updates branch 2 times, most recently from 77e505a to 31df0ac Compare August 21, 2018 19:24

alexanderbez approved these changes Aug 21, 2018

View reviewed changes

mslipper force-pushed the cli-updates branch from 31df0ac to 852cfd5 Compare August 21, 2018 21:32

jackzampolin changed the title ~~WIP: CLI UX Updates #1~~ R4R: CLI UX Updates #1 Aug 21, 2018

jackzampolin added ready-for-review and removed wip labels Aug 21, 2018

jaekwon reviewed Aug 21, 2018

View reviewed changes

Add plaintext flags to show keys; default print response to true; hel…

0c0d282

…ptext updates Closes cosmos#1970 Closes cosmos#1971 Closes cosmos#1967 Closes cosmos#1969

mslipper force-pushed the cli-updates branch from 852cfd5 to 0c0d282 Compare August 21, 2018 22:37

rigelrozanski added 2 commits August 21, 2018 23:23

Merge branch 'develop' into cli-updates

58f50c2

Merge branch 'develop' into cli-updates

a54e3a3

cwgoes reviewed Aug 22, 2018

View reviewed changes

Gopkg.lock

@@ -2,76 +2,57 @@

Copy link

Contributor

cwgoes Aug 22, 2018

Choose a reason for hiding this comment

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

Upgrade your version of dep, then this shouldn't happen

cwgoes added 2 commits August 22, 2018 12:40

Update Gopkg.lock

ba56c57

Merge branch 'develop' into cli-updates

42c03ae

cwgoes approved these changes Aug 22, 2018

View reviewed changes

cwgoes merged commit 29634bf into cosmos:develop Aug 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: CLI UX Updates #1 #1983

R4R: CLI UX Updates #1 #1983

mslipper commented Aug 11, 2018 •

edited

Loading

codecov bot commented Aug 11, 2018 •

edited

Loading

rigelrozanski commented Aug 11, 2018

ValarDragon commented Aug 11, 2018

ebuchman commented Aug 12, 2018

mslipper commented Aug 12, 2018

mslipper commented Aug 13, 2018

melekes Aug 14, 2018

alexanderbez Aug 14, 2018

melekes Aug 14, 2018

melekes Aug 14, 2018

mslipper Aug 17, 2018

melekes Aug 14, 2018

alexanderbez Aug 20, 2018

fedekunze Aug 19, 2018

alexanderbez left a comment

alexanderbez Aug 20, 2018

alexanderbez Aug 20, 2018

alexanderbez Aug 20, 2018

alexanderbez Aug 20, 2018

alexanderbez left a comment

jaekwon Aug 21, 2018

jaekwon Aug 21, 2018

jaekwon commented Aug 21, 2018

jackzampolin commented Aug 21, 2018

cwgoes Aug 22, 2018

cwgoes left a comment


		For example:

		$ gaiacli advanced tendermint txs --tag test1,test2

R4R: CLI UX Updates #1 #1983

R4R: CLI UX Updates #1 #1983

Conversation

mslipper commented Aug 11, 2018 • edited Loading

codecov bot commented Aug 11, 2018 • edited Loading

Codecov Report

rigelrozanski commented Aug 11, 2018

ValarDragon commented Aug 11, 2018

ebuchman commented Aug 12, 2018

mslipper commented Aug 12, 2018

mslipper commented Aug 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaekwon commented Aug 21, 2018

jackzampolin commented Aug 21, 2018

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

mslipper commented Aug 11, 2018 •

edited

Loading

codecov bot commented Aug 11, 2018 •

edited

Loading