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

Limit context background #8093

Merged
merged 14 commits into from
Feb 3, 2021
Merged

Limit context background #8093

merged 14 commits into from
Feb 3, 2021

Conversation

cyberbono3
Copy link
Contributor

Description

Found few cases for limiting context.Background()

Folllowing @robert-zaremba proposal:
-we can access context from function arguments or
-we can access context from parent function context

closes: #7834


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • 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
  • Review Codecov Report in the comment section below once CI passes

@codecov
Copy link

codecov bot commented Dec 5, 2020

Codecov Report

Merging #8093 (2c47549) into master (c1b567f) will decrease coverage by 0.01%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8093      +/-   ##
==========================================
- Coverage   61.50%   61.49%   -0.02%     
==========================================
  Files         651      651              
  Lines       37253    37266      +13     
==========================================
+ Hits        22912    22916       +4     
- Misses      11932    11945      +13     
+ Partials     2409     2405       -4     
Impacted Files Coverage Δ
client/keys/parse.go 76.27% <0.00%> (ø)
client/rpc/validators.go 0.00% <0.00%> (ø)
crypto/keyring/keyring.go 61.21% <ø> (ø)
client/keys/migrate.go 43.47% <41.66%> (-10.37%) ⬇️
x/slashing/client/cli/query.go 43.24% <66.66%> (ø)
x/staking/client/cli/query.go 73.27% <92.30%> (ø)
client/grpc/tmservice/block.go 50.00% <100.00%> (ø)
client/grpc/tmservice/service.go 65.45% <100.00%> (ø)
client/grpc/tmservice/status.go 50.00% <100.00%> (ø)
x/auth/client/cli/query.go 34.90% <100.00%> (ø)
... and 7 more

Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

Good start. However this PR doesn't fully close the issue. There are more places where we need to update the code to reduce context.Background use in non test files (eg in /types, /x/...)

client/rpc/validators.go Outdated Show resolved Hide resolved
client/rpc/validators.go Outdated Show resolved Hide resolved
client/rpc/validators.go Outdated Show resolved Hide resolved
@cyberbono3
Copy link
Contributor Author

Good start. However this PR doesn't fully close the issue. There are more places where we need to update the code to reduce context.Background use in non test files (eg in /types, /x/...)

Yeah, you are right. I have pushed the update.

Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

utACK

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Jan 23, 2021
@robert-zaremba robert-zaremba marked this pull request as ready for review January 23, 2021 09:45
@robert-zaremba
Copy link
Collaborator

Let's resolve conflicts and finish the review. Let me know if you can finish this PR.

@robert-zaremba robert-zaremba self-assigned this Jan 23, 2021
@github-actions github-actions bot closed this Jan 30, 2021
@robert-zaremba
Copy link
Collaborator

Let's reopen. @cyberbono3 -when are you planning to finish this?

Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

There are still few not resolved comments.

@cyberbono3 cyberbono3 force-pushed the limit-context branch 3 times, most recently from 1876128 to aa4c41d Compare February 1, 2021 03:57
@cyberbono3
Copy link
Contributor Author

@robert-zaremba all checks pass. Please take a look.

Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

looks good. thanks.

@@ -8,12 +8,12 @@ import (
"github.com/cosmos/cosmos-sdk/client"
)

func getBlock(clientCtx client.Context, height *int64) (*ctypes.ResultBlock, error) {
func getBlock(ctx context.Context, clientCtx client.Context, height *int64) (*ctypes.ResultBlock, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

in fact we are working on making client.Context implementing context.Context. But it's on hold now.

jgimeno and others added 2 commits February 2, 2021 09:59
Co-authored-by: Alessio Treglia <alessio@tendermint.com>
Co-authored-by: sahith-narahari <sahithnarahari@gmail.com>
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

thanks, utACK. I'm a bit suspicous why client/docs/statik/statik.go was modified, @cyberbono3 can you explain?

@amaury1093 amaury1093 added Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity. and removed stale labels Feb 2, 2021
@@ -62,7 +62,7 @@ func runMigrateCmd(cmd *cobra.Command, args []string) error {

var (
tmpDir string
migrator keyring.InfoImporter
migrator keyring.Importer
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm not sure why github is showing diff of another PR...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've seen it already in the past.

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Feb 3, 2021
@mergify mergify bot merged commit d37c590 into cosmos:master Feb 3, 2021
@robert-zaremba
Copy link
Collaborator

thanks, utACK. I'm a bit suspicous why client/docs/statik/statik.go was modified, @cyberbono3 can you explain?

I can respond on that. It inherits a context from command, rather than creating a new one.

@amaury1093
Copy link
Contributor

I can respond on that. It inherits a context from command, rather than creating a new one.

would prefer statik.go to be always generated, and never manually modified.

@robert-zaremba
Copy link
Collaborator

Ah, I didn't check the file is generated - good point. .... so we woould need to change the statik?

@cyberbono3
Copy link
Contributor Author

I have replaced modified statik.go with original statik.go from cosmos/master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limit the use of context.Background
6 participants