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

protoc-gen-go-grpc: remove use_generic_streams_experimental flag (defaults to true) #7654

Merged
merged 8 commits into from
Sep 26, 2024

Conversation

eshitachandwani
Copy link
Member

@eshitachandwani eshitachandwani commented Sep 20, 2024

Fixes task 2 : #1894 - Delete the code to generate stream interfaces for client and server.

Deletes the unused code to make stream generics default.

RELEASE NOTES: None

@eshitachandwani eshitachandwani added this to the 1.68 Release milestone Sep 20, 2024
Copy link

codecov bot commented Sep 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.76%. Comparing base (e7a8097) to head (df0fdb8).
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7654      +/-   ##
==========================================
- Coverage   81.80%   81.76%   -0.04%     
==========================================
  Files         361      361              
  Lines       27821    27821              
==========================================
- Hits        22758    22747      -11     
- Misses       3862     3867       +5     
- Partials     1201     1207       +6     

see 11 files with indirect coverage changes

@purnesh42H purnesh42H changed the title Protoc-gen-go-grpc:Delete the code to generate stream interface protoc-gen-go-grpc: delete the code to generate stream interface Sep 23, 2024
@purnesh42H
Copy link
Contributor

@eshitachandwani please try regenerating the protos and run the relevant examples to make sure everything is correct

@arvindbr8
Copy link
Member

@eshitachandwani please try regenerating the protos and run the relevant examples to make sure everything is correct

CI should already be doing that

} else {
s += method.Parent.GoName + "_" + method.GoName + "Client"
}

Copy link
Contributor

Choose a reason for hiding this comment

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

is 332 and 334 an empty line?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

Copy link
Member

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

changes LGTM, modulo two comments.

Also instead of saying partially fixes #issue, we should say which part of the issue is fixed. From the looks of it, it seems like the only thing pending is a release?

cmd/protoc-gen-go-grpc/grpc.go Outdated Show resolved Hide resolved
cmd/protoc-gen-go-grpc/grpc.go Outdated Show resolved Hide resolved
@arvindbr8 arvindbr8 changed the title protoc-gen-go-grpc: delete the code to generate stream interface protoc-gen-go-grpc: remove use_generic_streams_experimental flag (defaults to true) Sep 25, 2024
@eshitachandwani
Copy link
Member Author

Also instead of saying partially fixes #issue, we should say which part of the issue is fixed. From the looks of it, it seems like the only thing pending is a release?
Updating the gRPC.io to reflect the stream generics is also pending, but as far as grpc-go is concerned, yes only release is pending.

@eshitachandwani
Copy link
Member Author

@eshitachandwani please try regenerating the protos and run the relevant examples to make sure everything is correct
Done. It checks out correctly.

@purnesh42H purnesh42H self-requested a review September 25, 2024 15:27
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
Copy link
Contributor

@eshitachandwani please try regenerating the protos and run the relevant examples to make sure everything is correct

CI should already be doing that

that's great. good to know that

@purnesh42H
Copy link
Contributor

I think we need the release notes? @arvindbr8 to confirm

@arvindbr8
Copy link
Member

relnotes here are used by the release bot for gRPC releases only. relnotes for the plugin releases are handled manually.

@purnesh42H purnesh42H merged commit 3196f7a into grpc:master Sep 26, 2024
14 checks passed
janardhanvissa pushed a commit to janardhanvissa/grpc-go that referenced this pull request Oct 1, 2024
purnesh42H added a commit to purnesh42H/grpc-go that referenced this pull request Oct 7, 2024
dfawley pushed a commit that referenced this pull request Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants