Skip to content

feat(csharp/Reflection): change MessageParser.CreateTemplate to be public #19776

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ahmednfwela
Copy link

this is a subset of the changes in #7722 where MessageParser.CreateTemplate is made public, this removes the need to use reflection to create protobuf messages given a descriptor/parser.

future work: maybe for the next major release we can change the generated code to use MessageParser.CreateTemplate, fully solving #6401.

@ahmednfwela ahmednfwela requested a review from a team as a code owner December 23, 2024 00:07
@ahmednfwela ahmednfwela requested review from jskeet and removed request for a team December 23, 2024 00:07
@JasonLunn JasonLunn added c# 🅰️ safe for tests Mark a commit as safe to run presubmits over labels Dec 23, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Dec 23, 2024
@JasonLunn
Copy link
Contributor

Thank you for your contribution, @ahmednfwela. Although this PR is small, the impact of expanding the API surface could be significant and we need additional time to review it to make sure it aligns with our long term goals across the languages that Protobuf supports.

@esrauchg
Copy link

esrauchg commented Feb 4, 2025

Unfortunately our C# expertise is somewhat bandwidth limited right now, but as a lower-c#-context protobuf team member it seems a bit odd to me that this method should be a public method on Parser, since this is not really a parse related operation.

A similar shape of method is commonly an instance method on Message type, eg in Java var builder = msg.newBuilderForType() and in C++ the Message* m = msg.New() method (where there is a common pattern to pass around a SomeMsg.defaultInstance() as a prototype and then newBuilderForType()/New() on that in code that wants to create more instances of the same type).

Just to understand the possible design scope better, would a method on IMessage that is parallel to those precedents also solve for the need that you have?

@ahmednfwela
Copy link
Author

ahmednfwela commented Feb 4, 2025

@esrauchg
The whole point of this PR is to enable the creation of an arbitrary message given its parser (by exposing the factory method of the parser).

This opens the door for something like #7722 to unify the creation interface, so that the consumer can modify the factory logic to their needs (e.g. introducing reusable message pools).

I would personally not recommend moving the method on IMessage, because it forces us to have an instance of IMessage to be able to create an empty message .

But having it on the parser is similar to having an IMessageFactory which is a much better design

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants