Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Implement DialURLContext(...) #574

Merged
merged 5 commits into from
Sep 30, 2021
Merged

Conversation

cameronelliott
Copy link
Contributor

and convert DialURL to call DialURLContext()

and convert DialURL to call DialURLContext()
@cameronelliott
Copy link
Contributor Author

Hey @stevenh this is what I was thinking, although maybe there are reasons to pass the
context as an option, and it certainly would work functionally.

But it might confuse people.

I have never seen a context.Context passed as an option like that, and the Google
official manner is or was:

At Google, we require that Go programmers pass a Context parameter as the first argument to every function on the call path between incoming and outgoing requests.
Based upon this article:
https://go.dev/blog/context

And in my experience, the idiomatic Go way is to have a function named xxxContext when
you want to pass a context.Context in.

So, I might miss there is a way to pass a context.Context in if I didn't see
the method in the method signatures in the index, like in the typical Dial*Context method names.

But maybe it makes sense if the Dial method signatures are multiplying too much.

@stevenh
Copy link
Collaborator

stevenh commented Sep 26, 2021

Feels like we should change all Dialers to use ctx as the first arg in v2.

I believe xxxContent function naming is due to the lack of context support in early go versions + v1 compatibility guarantee, so the original functions must be kept.

Consistency vs other APIs which require ctx as first parameter is good point, it has the benefit that it enforces the design that a consumer should pass a valid context vs making less obvious with an additional functional option.

Copy link
Collaborator

@stevenh stevenh left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, I agree this is a better approach.

Lets get the docs for older none ctx Dial functions all updated to explicitly call out their wrap nature.

Please note this repo uses conventional commits.

redis/conn.go Show resolved Hide resolved
redis/conn.go Outdated Show resolved Hide resolved
Co-authored-by: Steven Hartland <steven.hartland@multiplay.co.uk>
@cameronelliott
Copy link
Contributor Author

Can you suggest/write the Conventional Commit labels?
I am not familiar with it.

@stevenh
Copy link
Collaborator

stevenh commented Sep 28, 2021

Can you suggest/write the Conventional Commit labels? I am not familiar with it.

Sure It's really simple this is a feature so you would have something like this:

feat: add DialURLContext

Add DialURLContext so that we can consumers have
control over cancelation and timeout

Add DialURLContext so that we can consumers have
control over cancelation and timeout
Copy link
Collaborator

@stevenh stevenh left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, just missing the suggested doc change for DialURL which makes it clear to the caller its a wrapper.

redis/conn.go Outdated Show resolved Hide resolved
Comments on DialURL should reflect it is a wrapper around DialURLContext
@stevenh stevenh merged commit bf63cd5 into gomodule:master Sep 30, 2021
@cameronelliott
Copy link
Contributor Author

@stevenh Thanks for taking this PR, and your patience while I got it into some type of passable shape.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants