Skip to content

clientv3: change Close() to close() for keepAlive and watchGrpcStream#8066

Merged
fanminshi merged 2 commits intoetcd-io:masterfrom
fanminshi:keepAlive_Close_to_close
Jun 8, 2017
Merged

clientv3: change Close() to close() for keepAlive and watchGrpcStream#8066
fanminshi merged 2 commits intoetcd-io:masterfrom
fanminshi:keepAlive_Close_to_close

Conversation

@fanminshi
Copy link
Copy Markdown
Contributor

@fanminshi fanminshi commented Jun 8, 2017

private structs such as keepAlive and watchGrpcStream shouldn't expose a public Close() method.

@xiang90
Copy link
Copy Markdown
Contributor

xiang90 commented Jun 8, 2017

Can you explain why we need this change in the commit message?

keepAlive is a private struct that belongs to clientv3 pkg and shouldn't expose a public Close() method.
@fanminshi fanminshi force-pushed the keepAlive_Close_to_close branch from f67fadb to 4dff7aa Compare June 8, 2017 18:54
@heyitsanthony
Copy link
Copy Markdown

this isn't being exported into godocs and there's no way to access this function, is this really fixing anything?

@fanminshi
Copy link
Copy Markdown
Contributor Author

@xiang90 added commit msg keepAlive is a private struct that belongs to clientv3 pkg and shouldn't expose a public Close() method.

@xiang90
Copy link
Copy Markdown
Contributor

xiang90 commented Jun 8, 2017

this isn't being exported into godocs and there's no way to access this function, is this really fixing anything?

it is an aesthetics change I believe. but I am ok with it as long as the commit message is clear.

@fanminshi
Copy link
Copy Markdown
Contributor Author

yeah, it makes code look better. Having a public method on a private struct doesn't seem right to me.

@heyitsanthony
Copy link
Copy Markdown

heyitsanthony commented Jun 8, 2017

there should be some kind of linting to find these if it's important

@heyitsanthony
Copy link
Copy Markdown

this patch should also cover watchGrpcStream.Close instead of doing it piecewise

@fanminshi
Copy link
Copy Markdown
Contributor Author

@heyitsanthony will add watchGrpcStream.Close () to this patch

private struct shouldn't have public method.
@fanminshi fanminshi changed the title clientv3: change keepAlive Close() to close() clientv3: change Close() to close() for keepAlive and watchGrpcStream Jun 8, 2017
@fanminshi fanminshi merged commit a8c073c into etcd-io:master Jun 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants