Skip to content

Conversation

sleipnir
Copy link
Collaborator

@sleipnir sleipnir commented May 9, 2025

This closes #270

@sleipnir sleipnir marked this pull request as ready for review May 9, 2025 19:57
@sleipnir
Copy link
Collaborator Author

sleipnir commented May 9, 2025

@polvalente, I think this is ready for review now. I would like to rename the modules:

GRPC.Server.Stream -> GRPC.Server.Materializer
and maybe their client-side equivalent.

Justification: I don't think they are a real Stream structure and in the new proposal they are only used to "materialize" the response to the client. It also looks a bit strange to have two modules named Stream in the library.
But I haven't gone ahead with this yet, waiting for your opinion.

The PR was a bit big but sorry there wasn't much that could be done since it's a likely breaking change.

Copy link
Contributor

@polvalente polvalente left a comment

Choose a reason for hiding this comment

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

Sorry about the lengthy review. This overall is looking good, but seeing that we're aiming for this to mark v1.0, I think we can commit to the breaking change of merging the GRPC.Stream and GRPC.Server.Stream structs, and remove other kinds of streaming, even if we have to do this through multiple PRs.

@sleipnir
Copy link
Collaborator Author

@polvalente shall we continue?

Copy link
Contributor

@polvalente polvalente left a comment

Choose a reason for hiding this comment

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

Just some minor comments and questions, but this looks mostly good to go (given the pending stuff we're leaving for follow-up PRs)

sleipnir and others added 9 commits May 14, 2025 16:36
Co-authored-by: Paulo Valente <[email protected]>
Co-authored-by: Paulo Valente <[email protected]>
Co-authored-by: Paulo Valente <[email protected]>
Co-authored-by: Paulo Valente <[email protected]>
Co-authored-by: Paulo Valente <[email protected]>
Co-authored-by: Paulo Valente <[email protected]>
Co-authored-by: Paulo Valente <[email protected]>
Co-authored-by: Paulo Valente <[email protected]>
Co-authored-by: Paulo Valente <[email protected]>
@sleipnir
Copy link
Collaborator Author

Just some minor comments and questions, but this looks mostly good to go (given the pending stuff we're leaving for follow-up PRs)

Great! I accepted all suggestions except two so we could discuss them further.

@sleipnir sleipnir merged commit 78b8386 into elixir-grpc:master May 14, 2025
7 checks passed
@sleipnir sleipnir deleted the feat/new-streams-api branch May 14, 2025 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New API Proposal
2 participants