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.

Sign up for GitHub

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

Jump to bottom

Change GetQueryCmd to take client.Context #6326

Merged
merged 5 commits into from
Jun 3, 2020
Merged

Change GetQueryCmd to take client.Context #6326

merged 5 commits into from
Jun 3, 2020

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Jun 2, 2020 •
edited
Loading

Description

ref: #5921

This makes GetQueryCmd similar to the new GetTxCmd which also takes client.Context now. It will be used for gRPC queriers.

For contributor use:

  • 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.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@aaronc aaronc added T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). R4R labels Jun 2, 2020
@aaronc aaronc marked this pull request as ready for review June 2, 2020 20:46
@codecov
Copy link

codecov bot commented Jun 2, 2020 •
edited
Loading

Codecov Report

Merging #6326 into master will not change coverage.
The diff coverage is 3.84%.

@@           Coverage Diff           @@
##           master    #6326   +/-   ##
=======================================
  Coverage   55.66%   55.66%           
=======================================
  Files         448      448           
  Lines       26956    26956           
=======================================
  Hits        15005    15005           
  Misses      10877    10877           
  Partials     1074     1074

alexanderbez
alexanderbez suggested changes Jun 3, 2020
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

One minor comment, other LGTM. Thanks!

types/module/module.go
@@ -52,8 +52,8 @@ type AppModuleBasic interface {

// client functionality
RegisterRESTRoutes(client.Context, *mux.Router)
GetTxCmd(client.Context) *cobra.Command
GetQueryCmd(*codec.Codec) *cobra.Command
GetTxCmd(clientCtx client.Context) *cobra.Command
Copy link
Contributor

Choose a reason for hiding this comment

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

nit the package and type suffice for context, no need to add a variable.

@alexanderbez alexanderbez added the A:automerge Automatically merge PR once all prerequisites pass. label Jun 3, 2020
alexanderbez
alexanderbez reviewed Jun 3, 2020
View reviewed changes
types/module/module.go
@@ -52,8 +52,8 @@ type AppModuleBasic interface {

// client functionality
RegisterRESTRoutes(client.Context, *mux.Router)
GetTxCmd(client.Context) *cobra.Command
GetQueryCmd(*codec.Codec) *cobra.Command
GetTxCmd(clientCtx client.Context) *cobra.Command
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
GetTxCmd(clientCtx client.Context) *cobra.Command
GetTxCmd(client.Context) *cobra.Command

alexanderbez
alexanderbez reviewed Jun 3, 2020
View reviewed changes
types/module/module.go
GetTxCmd(client.Context) *cobra.Command
GetQueryCmd(*codec.Codec) *cobra.Command
GetTxCmd(clientCtx client.Context) *cobra.Command
GetQueryCmd(clientCtx client.Context) *cobra.Command
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
GetQueryCmd(clientCtx client.Context) *cobra.Command
GetQueryCmd(client.Context) *cobra.Command

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding the variable name is actually helpful when implementing the method using code completion because we wanted clientCtx rather than ctx. But I can remove it if you prefer

Copy link
Contributor

Choose a reason for hiding this comment

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

Where would one get ctx from? I'd opt to remove it personally. Not blocking though, so proceed at your own discretion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry it would be context. I'd prefer to keep it because it helped my IDE (goland) keep the style conventions from #6290.

alexanderbez
alexanderbez approved these changes Jun 3, 2020
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

ACK

@mergify mergify bot merged commit fed0c23 into master Jun 3, 2020
@mergify mergify bot deleted the aaronc/5921-query-cmd branch June 3, 2020 20:15
@aaronc aaronc mentioned this pull request Jun 10, 2020
Closed
11 tasks
@clevinson clevinson added this to the v0.39 milestone Jun 11, 2020
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
@aaronc @alexanderbez
Change GetQueryCmd to take client.Context (cosmos#6326)
81980b3 
* Change GetQueryCmd to take a client.Context

* Update CHANGELOG

Co-authored-by: Alexander Bezobchuk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alessio alessio alessio approved these changes

@alexanderbez alexanderbez alexanderbez approved these changes

@clevinson clevinson Awaiting requested review from clevinson

@fedekunze fedekunze Awaiting requested review from fedekunze

@jgimeno jgimeno Awaiting requested review from jgimeno

Assignees
No one assigned
Labels
A:automerge Automatically merge PR once all prerequisites pass. T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).
Projects
None yet
Milestone
v0.40 [Stargate]
Development

Successfully merging this pull request may close these issues.
4 participants
@aaronc @alessio @alexanderbez @clevinson