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

Remove ContextDeriver and related functions #430

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

bluekeyes
Copy link
Member

This was added as a way to create a detatched context for async schedulers that still had some of the values of the original. Starting in Go 1.21, the standard library now provides the WithoutCancel function to make a copy of the values in a context but with a different lifetime.

Custom implementations of ContextDervier are likely very rare (if they exist at all) and their main purpose would be to copy context values other than the logger. Since WithoutCancel now copies all values, I don't see any reason to keep supporting custom functions.

While we could deprecate the existing functions, their rare usage combined with the fact that we frequently release breaking changes for go-github upgrades makes me believe this is safe.

Closes #428.

This was added as a way to create a detatched context for async
schedulers that still had some of the values of the original. Starting
in Go 1.21, the standard library now provides the WithoutCancel function
to make a copy of the values in a context but with a different lifetime.

Custom implementations of ContextDervier are likely very rare (if they
exist at all) and their main purpose would be to copy context values
other than the logger. Since WithoutCancel now copies all values, I
don't see any reason to keep supporting custom functions.

While we could deprecate the existing functions, their rare usage
combined with the fact that we frequently release breaking changes for
go-github upgrades makes me believe this is safe.
@bluekeyes
Copy link
Member Author

Note that this isn't causing any problems I'm aware of, but I was reading something unrelated last week and learned about context.WithoutCancel and thought we could use it here.

Copy link
Member

@asvoboda asvoboda left a comment

Choose a reason for hiding this comment

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

Nice find, its great to remove this code if we don't need to support it.

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.

Replace ContextDeriver with context.WithoutCancel
2 participants