-
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
examples: improve package comments #7658
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7658 +/- ##
==========================================
+ Coverage 81.86% 82.00% +0.14%
==========================================
Files 361 361
Lines 27821 27822 +1
==========================================
+ Hits 22775 22815 +40
+ Misses 3847 3825 -22
+ Partials 1199 1182 -17 |
@@ -16,7 +16,8 @@ | |||
* | |||
*/ | |||
|
|||
// Binary server is an example server. | |||
// Binary server demonstrates how to install and support compressors for | |||
// incoming RPCs. |
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 think its not clear how compressor is supported. Perhaps we can try to be a little bit more clear how support happens. may be at line 30?
// Importing the gzip encoding registers it as an available compressor.
// gRPC will automatically negotiate and use gzip if the client supports it.
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.
Done
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 think the current trailing comment on line 30 is clear enough and is more appropriate for a blank import compared to the comment added by this change.
A statement like // gRPC will automatically negotiate and use gzip if the client supports it.
feels more appropriate in the README file.
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.
oh I meant to modify the package comment to something like Binary server demonstrates importing the gzip encoding to register it as an available compressor
I remember an issue some time back where user was confused how to enable compression even after providing the link to example until I mentioned to them importing is all we need to do and rest is taken care by grpc
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.
Maybe, we should improve the documentation somewhere (either in the example, or in Documentation/compression.md
or in the gzip
package) then?
@@ -16,7 +16,8 @@ | |||
* | |||
*/ | |||
|
|||
// Binary server is an example server. | |||
// Binary server demonstrates how to install and support compressors for | |||
// incoming RPCs. |
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 think the current trailing comment on line 30 is clear enough and is more appropriate for a blank import compared to the comment added by this change.
A statement like // gRPC will automatically negotiate and use gzip if the client supports it.
feels more appropriate in the README file.
Updated the comments in various example files to provide clearer and more detailed descriptions and to make the code easier to understand for users who are reading through the examples.
Changes:
RELEASE NOTES: none