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

grpc: add docs for generic stream interfaces #7470

Merged
merged 26 commits into from
Aug 29, 2024

Conversation

janardhanvissa
Copy link
Contributor

@janardhanvissa janardhanvissa commented Aug 1, 2024

Partially addresses #1894

RELEASE NOTES: None

Copy link

codecov bot commented Aug 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.79%. Comparing base (ec9dff7) to head (6649fa6).
Report is 54 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7470      +/-   ##
==========================================
+ Coverage   81.56%   81.79%   +0.23%     
==========================================
  Files         354      361       +7     
  Lines       27076    27825     +749     
==========================================
+ Hits        22085    22760     +675     
- Misses       3802     3860      +58     
- Partials     1189     1205      +16     
Files with missing lines Coverage Δ
stream_interfaces.go 91.30% <ø> (ø)

... and 99 files with indirect coverage changes

stream_interfaces.go Outdated Show resolved Hide resolved
stream_interfaces.go Outdated Show resolved Hide resolved
stream_interfaces.go Outdated Show resolved Hide resolved
stream_interfaces.go Outdated Show resolved Hide resolved
@purnesh42H purnesh42H added the Type: Documentation Documentation or examples label Aug 2, 2024
@janardhanvissa
Copy link
Contributor Author

@purnesh42H Removed the comments for parent interfaces and pushed the changes.

@dfawley dfawley assigned purnesh42H and unassigned janardhanvissa Aug 6, 2024
Copy link
Contributor

@purnesh42H purnesh42H left a comment

Choose a reason for hiding this comment

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

LGTM

@purnesh42H purnesh42H added this to the 1.66 Release milestone Aug 7, 2024
@purnesh42H
Copy link
Contributor

@janardhanvissa can you please update the description that it partially addresses #1894 ? Also, add RELEASE NOTES to description. See other commits for reference

@janardhanvissa
Copy link
Contributor Author

@purnesh42H Partially in the sense do I need to add comments for RouteChat() function under examples/route_guide/server/server.go file?

@janardhanvissa
Copy link
Contributor Author

@purnesh42H Please review the Release note added in the PR.

@purnesh42H purnesh42H assigned dfawley and unassigned janardhanvissa Aug 7, 2024
@purnesh42H
Copy link
Contributor

@dfawley the documentation looks good to me. We can address the other follow up parts separately

Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Generally this feels a bit too verbose. Documentation shouldn't be redundant. Users reading this documentation know how to read the function signatures. They want to know what the things represent, how to use them, and what kinds of errors can occur. E.g. "can be called repeatedly" is great but "It takes a pointer ... of type Res" is obvious by reading the function signature. Please apply that feedback throughout and I will re-review (I stopped reviewing after the second interface).

stream_interfaces.go Outdated Show resolved Hide resolved
stream_interfaces.go Outdated Show resolved Hide resolved
stream_interfaces.go Outdated Show resolved Hide resolved
@purnesh42H purnesh42H assigned janardhanvissa and unassigned dfawley Aug 8, 2024
@janardhanvissa
Copy link
Contributor Author

@dfawley Updated the description as per the comments made. Please let me know if any updates are required for this.

@arvindbr8 arvindbr8 requested a review from dfawley August 8, 2024 23:12
@arvindbr8 arvindbr8 assigned dfawley and unassigned janardhanvissa Aug 8, 2024
@arvindbr8 arvindbr8 changed the title Adding Inline comments for stream interfaces in stream_interfaces.go grpc: add docs for generic stream interfaces Aug 21, 2024
@janardhanvissa
Copy link
Contributor Author

@arvindbr8 I have updated the docstring comments as per the comments addressed and pushed the changes. Please review it.

arvindbr8
arvindbr8 previously approved these changes Aug 26, 2024
@purnesh42H
Copy link
Contributor

@dfawley is this good to go?

@purnesh42H purnesh42H assigned dfawley and unassigned purnesh42H Aug 27, 2024
@dfawley
Copy link
Member

dfawley commented Aug 28, 2024

I will push a commit that has some edits soon, rather than going back and forth in comments.

Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

I think this looks good now. Please review my edits first, however.

@dfawley dfawley dismissed stale reviews from arvindbr8 and purnesh42H August 28, 2024 21:36

stale

@dfawley dfawley assigned purnesh42H and unassigned dfawley Aug 28, 2024
@purnesh42H purnesh42H merged commit 52961f7 into grpc:master Aug 29, 2024
14 checks passed
@janardhanvissa janardhanvissa deleted the genericStream-doc branch September 16, 2024 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Documentation Documentation or examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants