R4R reduce module interdependancy, /client refactor #4415

rigelrozanski · 2019-05-25T00:37:28Z

this is all just non-critical-breaking cleanup

Targeted PR against correct branch (see CONTRIBUTING.md)
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)

codecov · 2019-05-25T00:38:10Z

Codecov Report

Merging #4415 into master will decrease coverage by 0.83%.
The diff coverage is 48.27%.

@@            Coverage Diff            @@
##           master   #4415      +/-   ##
=========================================
- Coverage   56.23%   55.4%   -0.84%     
=========================================
  Files         244     243       -1     
  Lines       15411   15599     +188     
=========================================
- Hits         8666    8642      -24     
- Misses       6113    6330     +217     
+ Partials      632     627       -5

alessio · 2019-05-27T13:11:03Z

I like to idea of reorganizing client! Can we have a brief recap/summary of what this PR is about please?

sabau · 2019-05-27T13:12:29Z

May I ask a little PR description, just to understand the goals it achieves? [I've seen now that this is a duplicated message]

alessio · 2019-05-27T16:26:08Z

I think I got a handle on this, and I like:

the new flag package: we should have done it long ago - better late than never, thus
the new input package - looks nice and clean, naming is consistent and self-explanatory

What I don't like/am not sure about:

Why should the top level client package export flags variables as aliases? Wouldn't it be more appropriate to expect the caller to import the flags package instead?
same goes mostly for all sub packages TBH

alessio

I'm not entirely convinced about aliasing all client sub packages public members. However, I don't think this should block on such small thing either way. Thus, ACK

jackzampolin · 2019-05-27T17:49:18Z

client/keys/add.go

@@ -62,13 +66,13 @@ the flag --nosort is set.
 	cmd.Flags().Bool(flagNoSort, false, "Keys passed to --multisig are taken in the order they're supplied")
 	cmd.Flags().String(FlagPublicKey, "", "Parse a public key in bech32 format and save it to disk")
 	cmd.Flags().BoolP(flagInteractive, "i", false, "Interactively prompt user for BIP39 passphrase and mnemonic")
-	cmd.Flags().Bool(client.FlagUseLedger, false, "Store a local reference to a private key on a Ledger device")
+	cmd.Flags().Bool(flags.FlagUseLedger, false, "Store a local reference to a private key on a Ledger device")


So I love the idea of the flags package, but what about making the flags in that package pflag.Flags. This would allow us to pass the cmd to a function which adds all the necessary client.Flag*s to the cmd.FlagSet. See:

https://godoc.org/github.com/spf13/pflag#Flag
https://godoc.org/github.com/spf13/cobra#Command.Flags
https://godoc.org/github.com/spf13/pflag#FlagSet.AddFlag
https://godoc.org/github.com/spf13/pflag

oh yeah we should probably rename the packages to not conflict with the flags namespace... maybe?

Yes, that's exactly what we should be doing - I'd not block this PR though

@jackzampolin I think I misunderstood you when I made my first comment. In regards to using pflags we already do unless I'm missing something. There are only 2 places in the entire repo were we import regular flags (simapp/sim_test.go and contrib/runsim/main.go)

jack [2:44 PM]
Oh I was saying that we could refactor all of the client.Flag* defs (currently string type) to be *pflag.Flag type

which would be a separate PR
Then when registering them we could pass an []*pflag.Flag{} to the cmd
And the client/flags package could potentially have some standard groupings
Anyway, not a big deal
It would be a separate PR.
Any thoughts on that?

fp4k [2:50 PM]
oh I see, yeah well we wouldn’t want to remove the string (because we still need it to access the flag value using viper) but we could include an additional variable which is the actual pflag definition. I think that makes sense as an additional PR as you’ve mentioned

jack [2:50 PM]
Well the string would be under pflag.Flag.Name

fp4k [2:50 PM]
oh very good 👏

rigelrozanski · 2019-05-27T18:02:39Z

@alessio

Why should the top level client package export flags variables as aliases? Wouldn't it be more appropriate to expect the caller to import the flags package instead?
same goes mostly for all sub packages TBH

from my experience of digesting the code base, I think it's often more succinct to import one package with alias, instead of several of the root subpackages. This especially comes into play as multiple subpackages from different high level packages have duplicate package names (for example client/flags and yourmodule/flags. When this scenario arises, the developer is forced to introduce a moniker name for the import (for ex. clientflags and yourmoduleflags) to distinguish the two packages which becomes verbose. Additionally, often times, I'm not specifically concerned about which subpackage the import came from, I'm more interested in the high level import path (aka client/). In some files we have I've seen exorbitant numbers of imports which make it quite difficult to grok without having to constantly refer to the import block at the top of the file, by only importing the high level packages we eliminate that burden. Also - on the few occasions that I do what to know more specifics about the subpackages, I simply go to the definition, where I see the subpackage import.

Lastly I will mention, that although I think it's usually preferable to import the high level packages, there are some occasions where importing the subpackage is required do import cycles (which is often why the subpackages are introduced to begin with!). And also there are times, when an import is only referring to a single subpackage from an import package, In this situation, depending on the absence of conflicting import names, it may desirable to import the subpackage instead of the high level package.

So for all the above reasons, I think it's valuable to allow for the import of aliases, but not enforce that this is a required style, as sometimes (as explained in the second paragraph) it's actually more desirable or required to import the subpackage. As such I have not changed many of the files in the sdk which import sub-packages during this client refactor, although maybe some of them should be changed to only import the high level client package.

capisce?

alessio · 2019-05-27T18:11:25Z

@rigelrozanski developers convenience would have been enough. As I said above, it's not a big deal - I confirm my nihil obstat.

jackzampolin

Good reorganization of the client/flags and client/input code as well as related refactors. LGTM 👍

fedekunze

LGTM

alexanderbez · 2019-05-28T12:56:42Z

client/rpc/root.go

@@ -13,7 +13,7 @@ import (
 )

 // Register REST endpoints
-func RegisterRoutes(cliCtx context.CLIContext, r *mux.Router) {
+func RegisterRPCRoutes(cliCtx context.CLIContext, r *mux.Router) {


We shouldn't have done this tbh. The package is self describing. Similar to just like you don't do NewFoo in package foo.

This is a downside of aliasing sub packages functions at top level: you need to come up with unique names at every turn. I merged it anyway because it was consistent with @rigelrozanski's view on this.

go.mod

rigelrozanski added 4 commits May 23, 2019 16:01

abstract staking BuildCreateValidatorMsg, genutil defines its own flags

6f89645

client/ refactor

50ca1bc

staking move keys from keeper to types

3594ed9

...

6198c4d

rigelrozanski requested review from alessio, alexanderbez and jackzampolin as code owners May 25, 2019 00:37

pending

2f01311

rigelrozanski mentioned this pull request May 25, 2019

update gaia for various sdk updates cosmos/gaia#22

Closed

5 tasks

alessio requested review from sabau and fedekunze May 27, 2019 13:11

alessio approved these changes May 27, 2019

View reviewed changes

rigelrozanski changed the title ~~reduce module interdependancy, /client refactor~~ R4R reduce module interdependancy, /client refactor May 27, 2019

jackzampolin reviewed May 27, 2019

View reviewed changes

jackzampolin approved these changes May 27, 2019

View reviewed changes

fedekunze approved these changes May 27, 2019

View reviewed changes

alessio merged commit 73e5ef7 into master May 28, 2019

alessio deleted the rigel/rm-module-interdeps branch May 28, 2019 08:44

alexanderbez reviewed May 28, 2019

View reviewed changes

go.mod Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R reduce module interdependancy, /client refactor #4415

R4R reduce module interdependancy, /client refactor #4415

rigelrozanski commented May 25, 2019 •

edited

Loading

codecov bot commented May 25, 2019

alessio commented May 27, 2019

sabau commented May 27, 2019 •

edited

Loading

alessio commented May 27, 2019

alessio left a comment

jackzampolin May 27, 2019 •

edited

Loading

rigelrozanski May 27, 2019

alessio May 27, 2019

rigelrozanski May 27, 2019

rigelrozanski May 27, 2019 •

edited

Loading

rigelrozanski commented May 27, 2019 •

edited

Loading

alessio commented May 27, 2019

jackzampolin left a comment

fedekunze left a comment

alexanderbez May 28, 2019 •

edited

Loading

alessio May 28, 2019

R4R reduce module interdependancy, /client refactor #4415

R4R reduce module interdependancy, /client refactor #4415

Conversation

rigelrozanski commented May 25, 2019 • edited Loading

codecov bot commented May 25, 2019

Codecov Report

alessio commented May 27, 2019

sabau commented May 27, 2019 • edited Loading

alessio commented May 27, 2019

alessio left a comment

Choose a reason for hiding this comment

jackzampolin May 27, 2019 • edited Loading

Choose a reason for hiding this comment

rigelrozanski May 27, 2019

Choose a reason for hiding this comment

alessio May 27, 2019

Choose a reason for hiding this comment

rigelrozanski May 27, 2019

Choose a reason for hiding this comment

rigelrozanski May 27, 2019 • edited Loading

Choose a reason for hiding this comment

rigelrozanski commented May 27, 2019 • edited Loading

alessio commented May 27, 2019

jackzampolin left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

alexanderbez May 28, 2019 • edited Loading

Choose a reason for hiding this comment

alessio May 28, 2019

Choose a reason for hiding this comment

rigelrozanski commented May 25, 2019 •

edited

Loading

sabau commented May 27, 2019 •

edited

Loading

jackzampolin May 27, 2019 •

edited

Loading

rigelrozanski May 27, 2019 •

edited

Loading

rigelrozanski commented May 27, 2019 •

edited

Loading

alexanderbez May 28, 2019 •

edited

Loading