objective-c: Add default_objc_class_prefix generation option.

[dnkoutso]

For #9469

[thomasvl]

The fact that developers have to edit the files for the prefix has always sorta bugged me, so I'm not opposed to this. Just want to make sure it is something we spec that can be supported long term.

I wonder if maybe we should tweak your proposed generation option name to be default_objc_class_prefix to better indicate when/how it would be used?

[dnkoutso]

I wonder if maybe we should tweak your proposed generation option name to be default_objc_class_prefix to better indicate when/how it would be used?

Yeah I did not like it myself either and I appreciate this take. I think we should change it to default_objc_class_prefix as it can easily differentiate it from the file option.

[dnkoutso]

@thomasvl here's a thought:

Should we actually make the objc_opt option be honored first and before the file option, if it is specified? I assume if we ship this we expect folks to be able to use it without the need to update their proto files to remove the file option in order to be able to use it. I do not think doing that will cause any breaking changes to existing protoc invocations as it will require someone to explicitly pass the new generator option.

If we do make it first, should not name it something like default_... and perhaps something else?

[thomasvl]

@thomasvl here's a thought:

Should we actually make the objc_opt option be honored first and before the file option, if it is specified? I assume if we ship this we expect folks to be able to use it and will have to update their proto files to remove the file option in order to be able to use this new one.

I was debating that after reading the issue and starting to look at your PR. We could continue with the current PR and making it a default, and that doesn't prevent us from later adding a forced_objc_class_prefix (or maybe override_objc_class_prefix), but I'm not sure it has to be done at the same time.

I can see values for both, again, once it is officially provided, it has to be supported, so we'll need to think through if there are down sides to this…

Since the prefixes can be needed to avoid type collisions, I can see something that is an override becoming a foot gun when some .proto author has two packages with a User message, and all languages using proto packages keep them unique, and that author might have even included ObjC prefixes to keep them unique, but this generation option can't be used because it completely defeats the proto authors work to help developers out.

So yes, there are likely lots of cases it is safe and someone could use it, but then suddenly one day they get a new proto dependency or one gets updated, and they suddenly have to revise everything because we've lead them down a path that is sorta asking for this issue in the future.

Again, not completely opposed, but as an override, it likely need to be thought about a bit longer compared to a default one.

[dnkoutso]

I was debating that after reading the issue and starting to look at your PR. We could continue with the current PR and making it a default, and that doesn't prevent us from later adding a forced_objc_class_prefix (or maybe override_objc_class_prefix), but I'm not sure it has to be done at the same time.

Sounds good! I will proceed with renaming it to default and update README etc.

[dnkoutso]

@thomasvl updated to use default_objc_class_prefix generator option along with all other comments.

Please let me know if i should update anything else and thank you for the review and guidance!

[dnkoutso]

@thomasvl I looked into adding some tests...it seems it should be possible, let me know if its a requirement

[dnkoutso]

Looks like one shard flaked?

[thomasvl]

@thomasvl I looked into adding some tests...it seems it should be possible, let me know if its a requirement

I'm ok without a specific test on this given the simplicity of it.

Looks like one shard flaked?

Yea, some issue not related to your PR.

[dnkoutso]

thank you!

[dnkoutso]

@thomasvl this is actually causing issues.

It turns out that g_prefix_mode is a "global" mode for deriving objc class prefix. This means that when a proto depends on on another proto we are outputting the incorrect prefix for dependencies.

Do you have a place to store this for each proto file options (but not real proto file options)

[dnkoutso]

I guess I can build it into g_prefix_mode but keep track of each file similarly to is_package_exempted

[dnkoutso]

updating this now.

[dnkoutso]

maybe this can be rearchitected into a file like require_prefixes because we can pass multiple proto files for generation and default_objc_class_prefix does not make sense?

[dnkoutso]

Going to update this to be based from a file instead. The file can be

:proto

If one wants to use a "default" one they can just output the same prefix for all files.

[dnkoutso]

This was reverted here #9532 and replaced with a new solution here #9498