Skip to content
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

Make an SDK function for getting base module query command command #11867

Closed
4 tasks
ValarDragon opened this issue May 3, 2022 · 8 comments
Closed
4 tasks

Comments

@ValarDragon
Copy link
Contributor

Summary

Every module has roughly the same few lines for getting the base cobra command. So much so, that ignite basically code generates boilerplate for this. Lets just make a common function in the SDK that every module can use, and save ourselves these extra 6 lines per module?

Staking cli query base: https://github.com/cosmos/cosmos-sdk/blob/main/x/staking/client/cli/query.go#L19-L25

   stakingQueryCmd := &cobra.Command{
   	Use:                        types.ModuleName,
   	Short:                      "Querying commands for the staking module",
   	DisableFlagParsing:         true,
   	SuggestionsMinimumDistance: 2,
   	RunE:                       client.ValidateCmd,
   }

Proposal

I propose we make a method that does this generically, taking in types.ModuleName as argument, and using that in Use and in Short (via fmt.SprintF)

Then we change this in every module to be:

stakingQueryCmd = somePackage.BaseModuleQueryCommand(types.ModuleName)

I don't know which package this belongs in, perhaps client?


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alexanderbez
Copy link
Contributor

@aaronc are you also working on refactoring this lol?

@aaronc
Copy link
Member

aaronc commented May 4, 2022

@ValarDragon see the work that is started in client/v2 - that should simplify a lot creating CLI commands. However, I'm not really sure ModuleName is where we should make the simplifications. I will need to write some ADRs for both this and the app wiring work so the designs are more centralized

@ValarDragon
Copy link
Contributor Author

ah amazing, didn't know about that! Just saw the ADR, lets close this in favor of that work for just generating these CLI files automatically.

@ValarDragon
Copy link
Contributor Author

Went ahead and did this for Osmosis osmosis-labs/osmosis#3611

@tac0turtle tac0turtle reopened this Dec 3, 2022
@tac0turtle
Copy link
Member

Reopening this in order to track Autocli and potentially up streaming osmosis work

@saif-xgrid
Copy link

Hey folks. Is this issue still open ?

@julienrbrt
Copy link
Member

Hey folks. Is this issue still open ?

AutoCLI solves that issue and is being tackled here: #11775.

@julienrbrt
Copy link
Member

Given that AutoCLI is now getting implemented in the SDK closing this.
OsmoCLI is definitely another interesting implementation, but still more verbose than AutoCLI aims to be.
Tracking is done #11775.
It would be great to have Osmosis try out AutoCLI when you migrate to v0.50 and tell us if something is missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants