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

compiler: add option @generated=omit #11086

Merged
merged 3 commits into from
Apr 19, 2024

Conversation

panchenko
Copy link
Contributor

@panchenko panchenko commented Apr 8, 2024

related to #11081

@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Apr 11, 2024
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Apr 11, 2024
@alexanderankin
Copy link
Contributor

@panchenko is this backwards compatible, as people are already using this?

@alexanderankin
Copy link
Contributor

It doesn't look like it to me

@panchenko
Copy link
Contributor Author

@panchenko is this backwards compatible, as people are already using this?

The option with an unclear name has been released just a few days ago, it can be discussed if it's worth keeping it compatible, personally I don't think so.
IMHO can be handled like a "typo".

@alexanderankin
Copy link
Contributor

@panchenko - even so, you have examples of people already using it in the thread, and many others who got notified on several email threads (not just this repo) about the issue being closed (and reopened).

besides that on merit, the name jakarta makes more sense as this is what new programmers are coming in learning about. the name javax doesn't mean anything anymore, jakarta_include (which is unlikely to be implemented but one can hope), jakarta_omit, and jakarta_javax make sense, but including = does not because these are not key value pairs, they are really just int argc, char *argv[], which is misleading to users of the library, which, since its java, are just people at work whose lives are already made miserable without our help

Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

I like this. I also wasn't wild about the option name and delayed string checking before.

I have some concern of using @ at the beginning of an option, as @filename is special in protoc and elsewhere. But it looks like it'd be fine.

@ejona86
Copy link
Member

ejona86 commented Apr 11, 2024

panchenko is this backwards compatible, as people are already using this?

I'm not too worried about backward compatibility because the users setting this are also in control of the protoc-gen-grpc version. It's also only been around a single release.

name jakarta makes more sense as this is what new programmers are coming in learning about

The chance we ever use the Jakarta classes here is close to 0%.

the name javax doesn't mean anything anymore

It is literally in the package name for the class. And that's also only because you are aware of what is and is not part of Jakarta. javax itself is still very much alive: javax.crypto, javax.net, javax.xml....

including = does not because these are not key value pairs, they are really just int argc, char *argv[]

That is identical to saying a command line argument --foo=bar doesn't make sense, even though it is frequently used. The options are weird in protobuf as they were bolted-on, but using equals is standard (just like how --baz or --foo=bar are both normal). https://github.com/protocolbuffers/protobuf/blob/da651cd3c286d0f2206f13947d96422370fa5ccc/src/google/protobuf/compiler/code_generator.h#L224

@sergiitk sergiitk changed the title option '@generated=omit' compiler: add option @generated=omit Apr 19, 2024
@sergiitk sergiitk merged commit 8a21afc into grpc:master Apr 19, 2024
13 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants