R4R: Ensure all CLI queries respect output flags #3320

jackzampolin · 2019-01-18T00:58:59Z

There were also some small changes to 3 staking commands to remove excessive flags and replace them with arguments. Each of these commands were the singular version (redelegation vs redelegations) where the plural took no flags, and the singular took 2-3. This PR results in a much cleaner and consistent UX for queries on the command line.

I've also added some comments here to help reviewers.

Wrote tests
Updated relevant documentation (docs/)
Added entries in PENDING.md with issue #
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)

codecov · 2019-01-18T01:08:48Z

Codecov Report

Merging #3320 into develop will decrease coverage by 0.71%.
The diff coverage is 3.59%.

@@             Coverage Diff             @@
##           develop    #3320      +/-   ##
===========================================
- Coverage    55.64%   54.92%   -0.72%     
===========================================
  Files          132      132              
  Lines         9559     9680     +121     
===========================================
- Hits          5319     5317       -2     
- Misses        3903     4026     +123     
  Partials       337      337

fedekunze

some minor comments

PENDING.md

docs/gaia/gaiacli.md

client/context/context.go

x/gov/client/utils/query.go

x/staking/types/validator.go

client/context/context.go

x/auth/account.go

x/gov/proposals.go

client/tx/search.go

x/gov/client/utils/query.go

x/auth/account.go

x/staking/types/delegation.go

x/staking/types/validator_test.go

alessio

Comments addressed, thus nihil obstat from me.

alexanderbez

Looks great @jackzampolin! I left some initial feedback, but have not manually tested yet. I will be doing that shortly.

cmd/gaia/cli_test/test_helpers.go

x/auth/account.go

x/distribution/client/cli/query.go

alexanderbez · 2019-01-21T17:42:09Z

x/distribution/types/validator.go

+
+func (vs ValidatorSlashEvents) String() string {
+	out := "Validator Slash Events:\n"
+	for i, sl := range vs {


Should these have newlines appended?

They are actually multiline strings and the newlines are added properly. Are you seeing issues when using that function?

Im just thinking how will multiple slashes be separated?

x/gov/client/utils/query.go

x/gov/depositsvotes.go

x/staking/types/delegation.go

alexanderbez

I also noticed some decimal values are stringified and others are not (e.g. gov vs distr)

alexanderbez · 2019-01-22T14:49:42Z

cmd/gaia/cli_test/test_helpers.go

 	cmd := fmt.Sprintf("gaiacli query gov proposals %v", f.Flags())
 	stdout, stderr := tests.ExecuteT(f.T, addFlags(cmd, flags), "")
+	if strings.Contains(stderr, "No matching proposals found") {


Thanks @jackzampolin -- we should use error variables in the future (e.g. strings.Contains(stderr, gov.ErrNoMatchingProposals)

Absolutely. There is a bunch more that needs to be done to standardize errors across the various client software.

alexanderbez · 2019-01-22T15:04:58Z

x/gov/client/utils/query.go

+}
+
+func (p Proposer) String() string {
+	return fmt.Sprintf("Proposal w/ ID %d was proposed by %s", p.ProposalID, p.Proposer)


Suggested change

return fmt.Sprintf("Proposal w/ ID %d was proposed by %s", p.ProposalID, p.Proposer)

return fmt.Sprintf("Proposal with ID %d was proposed by %s", p.ProposalID, p.Proposer)

alexanderbez · 2019-01-22T15:05:17Z

x/gov/client/utils/query.go

+
+// QueryProposalByID takes a proposalID and returns a proposal
+func QueryProposalByID(proposalID uint64, cliCtx context.CLIContext, cdc *codec.Codec, queryRoute string) ([]byte, error) {
+	// Construct query


Suggested change

// Construct query

cwgoes · 2019-01-22T18:02:58Z

PENDING.md

@@ -37,10 +33,18 @@ BREAKING CHANGES
  * [\#3064](https://github.com/cosmos/cosmos-sdk/issues/3064) Sanitize `sdk.Coin` denom. Coins denoms are now case insensitive, i.e. 100fooToken equals to 100FOOTOKEN.
  * [\#3195](https://github.com/cosmos/cosmos-sdk/issues/3195) Allows custom configuration for syncable strategy
  * [\#3242](https://github.com/cosmos/cosmos-sdk/issues/3242) Fix infinite gas
+<<<<<<< HEAD


@jackzampolin this doesn't look intentional - merge artifact?

can we take care of this in the release PR? 😬 sorry about this

jackzampolin requested review from cwgoes, ebuchman, rigelrozanski and zramsay as code owners January 18, 2019 00:59

This was referenced Jan 18, 2019

R4R: Allow overriding of CLI transaction response handling #3276

Closed

WIP: CLI Response Formatting #3301

Closed

fedekunze reviewed Jan 18, 2019

View reviewed changes

PENDING.md Outdated Show resolved Hide resolved

docs/gaia/gaiacli.md Show resolved Hide resolved

client/context/context.go Show resolved Hide resolved

x/gov/client/utils/query.go Outdated Show resolved Hide resolved

x/staking/types/validator.go Show resolved Hide resolved

jackzampolin mentioned this pull request Jan 18, 2019

R4R: CLIContext Refactor #3321

Closed

7 tasks

jackzampolin force-pushed the jack/cli-outputs branch from 872893c to f2ea39c Compare January 18, 2019 17:15

jackzampolin added C:CLI ready-for-review T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). labels Jan 18, 2019

jackzampolin force-pushed the jack/cli-outputs branch from f2ea39c to b6bea2b Compare January 18, 2019 21:37

jackzampolin commented Jan 18, 2019

View reviewed changes

client/context/context.go Show resolved Hide resolved

jackzampolin commented Jan 18, 2019

View reviewed changes

x/auth/account.go Show resolved Hide resolved

jackzampolin commented Jan 18, 2019

View reviewed changes

x/gov/proposals.go Show resolved Hide resolved

alessio reviewed Jan 18, 2019

View reviewed changes

client/tx/search.go Show resolved Hide resolved

alessio reviewed Jan 19, 2019

View reviewed changes

alessio approved these changes Jan 21, 2019

View reviewed changes

alexanderbez suggested changes Jan 21, 2019

View reviewed changes

alexanderbez reviewed Jan 21, 2019

View reviewed changes

jackzampolin mentioned this pull request Jan 21, 2019

Use templates for fmt.Stringer implementation #3340

Closed

alexanderbez reviewed Jan 22, 2019

View reviewed changes

alexanderbez approved these changes Jan 22, 2019

View reviewed changes

jackzampolin added 5 commits January 22, 2019 08:56

Ensure all CLI queries respect output flags

ad4764d

Pending.md

0c783e3

Address @fedekunze PR comments

4d3f199

Update PR to address distr routes as well

f961a70

Address @alessio PR comments

562e57b

Address @alexanderbez PR comments'

fbcd835

jackzampolin force-pushed the jack/cli-outputs branch from a73ba9b to fbcd835 Compare January 22, 2019 16:57

jackzampolin added 2 commits January 22, 2019 08:59

Address @alexanderbez PR comments

69fe4ce

Fix pool issue

8262553

alexanderbez approved these changes Jan 22, 2019

View reviewed changes

jackzampolin merged commit 9f50c9f into develop Jan 22, 2019

cwgoes reviewed Jan 22, 2019

View reviewed changes

rigelrozanski deleted the jack/cli-outputs branch February 20, 2019 06:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Ensure all CLI queries respect output flags #3320

R4R: Ensure all CLI queries respect output flags #3320

jackzampolin commented Jan 18, 2019 •

edited

Loading

codecov bot commented Jan 18, 2019 •

edited

Loading

fedekunze left a comment

alessio left a comment

alexanderbez left a comment

alexanderbez Jan 21, 2019

jackzampolin Jan 21, 2019

alexanderbez Jan 22, 2019

alexanderbez left a comment

alexanderbez Jan 22, 2019

jackzampolin Jan 22, 2019

alexanderbez Jan 22, 2019

alexanderbez Jan 22, 2019

cwgoes Jan 22, 2019

jackzampolin Jan 22, 2019

	return fmt.Sprintf("Proposal w/ ID %d was proposed by %s", p.ProposalID, p.Proposer)
	return fmt.Sprintf("Proposal with ID %d was proposed by %s", p.ProposalID, p.Proposer)

R4R: Ensure all CLI queries respect output flags #3320

R4R: Ensure all CLI queries respect output flags #3320

Conversation

jackzampolin commented Jan 18, 2019 • edited Loading

codecov bot commented Jan 18, 2019 • edited Loading

Codecov Report

fedekunze left a comment

Choose a reason for hiding this comment

alessio left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackzampolin commented Jan 18, 2019 •

edited

Loading

codecov bot commented Jan 18, 2019 •

edited

Loading