-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
godoc: clarify WithTimeout deprecation note #3226
Conversation
FYI, the travis failure looks unrelated to me, but I don't see how I can ask it to rerun. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification.
dialoptions.go
Outdated
@@ -374,8 +374,8 @@ func init() { | |||
// is returned by f, gRPC checks the error's Temporary() method to decide if it | |||
// should try to reconnect to the network address. | |||
// | |||
// Deprecated: use WithContextDialer instead. Will be supported throughout | |||
// 1.x. | |||
// Deprecated: use WithContextDialer or DialContext instead of Dial. Will be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect. WithContextDialer
is to be used instead of WithDialer
itself. I don't think any change here is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. I finally found a working way to port from the deprecated api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this is still incorrect. If you port from grpc.Dial() to grpc.DialContext you don't need WithContextDialer() at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WithContextDialer
is the new version of WithDialer
. DialContext
is the new version of Dial
(though Dial
itself is not deprecated since a context isn't useful without WithBlock
). These are entirely orthogonal.
dialoptions.go
Outdated
@@ -46,7 +46,7 @@ type dialOptions struct { | |||
chainUnaryInts []UnaryClientInterceptor | |||
chainStreamInts []StreamClientInterceptor | |||
|
|||
cp Compressor | |||
cp Compressor* t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't change this. I'll drop the PR:
dialoptions.go
Outdated
@@ -374,8 +374,7 @@ func init() { | |||
// is returned by f, gRPC checks the error's Temporary() method to decide if it | |||
// should try to reconnect to the network address. | |||
// | |||
// Deprecated: use WithContextDialer instead. Will be supported throughout | |||
// 1.x. | |||
// Deprecated: use WithContextDialer instead. Will be supported throughout 1.x. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please re-wrap this to avoid the unnecessary diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why you want 2 spaces separating the sentence in the docs. But anyway done.
Tell people to replace Dial + WithTimeout by DialContext + context.WithTimeout.
Tell people to replace Dial + WithTimeout by DialContext + context.WithTimeout.