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

Extra linters to address common review points. #411

Merged
merged 5 commits into from
Aug 24, 2023

Conversation

jholdstock
Copy link
Member

Some of these cover points which have recently been highlighted in reviews, such as contexts contained in structs, slices initialized with zero length, and periods missing from the end of comments.

Documentation for the context package suggests that Contexts should
never be stored in structs.
Copy link
Member

@davecgh davecgh 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 overall.

I understand this is focusing on removing the contexts from the struct, so nothing to hold the PR up over, but to offer a suggestion for future PRs, pretty much all of the cases that are wrappers for the RPCs (e.g. GetRawTransaction) should really be taking contexts so that each individual call can be independently cancelled and traced (if desired) as that is kind of the entire purpose of contexts in the RPCs.

Generally speaking, the callers to those functions would create a new context that wraps the overall shutdown context (or the already wrapped context for a subsystem, etc) to ensure the they are cancelled on shutdown due to context cancellation propagation.

For example, they might do context.WithTimeout(shutdownCtx, time.Second * 5) or context.WithDeadline(shutdownCtx, ...) etc.

Some of these cover points which have recently been highlighted in
reviews, such as contexts contained in structs, slices initialized with
zero length, and periods missing from the end of comments.
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

Reviewed the latest commit to add more linters too.

@jholdstock jholdstock merged commit fe8e63a into decred:master Aug 24, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants