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

Handle field names of "descriptor" and "types". #665

Merged
merged 1 commit into from Jul 31, 2015

Conversation

jskeet
Copy link
Contributor

@jskeet jskeet commented Jul 31, 2015

master equivalent of #662. Merging as that has an LG (when the build is green).

jskeet added a commit that referenced this pull request Jul 31, 2015
Handle field names of "descriptor" and "types".
@jskeet jskeet merged commit 5bdb1fb into protocolbuffers:master Jul 31, 2015
@jskeet jskeet deleted the types_field branch July 31, 2015 07:13
@agoodwin
Copy link

Hi @jskeet,

I just made a reference to this issue from this one: grpc/grpc#31590.

I was just wondering, do you remember (it's a long shot, I know) when you made this change why Types was a member name that needed to be handled?

I recently attempted to have a types property in one of my protobuf messages, but when I saw it being generated in C# with the _ suffix (Types_), I decided I'd prefer to find a different, slightly suboptimal name, rather than have the "ugliness" of the suffix.

Having said that, I couldn't see any reason why Types would have caused a naming collision, so I was curious if this is a case that doesn't actually need to be restricted? Was it just something needed e.g. with a previous version of protobuf?

I realise that even if it's no longer the case that it's needed, it's going to be an extremely low priority thing to change, and the ship has probably already sailed on that anyway as I guess it would be a breaking change now.

Still, if you do remember off the top of your head what this was needed for that'd be great, otherwise please feel to ignore this.

Thanks

@jskeet
Copy link
Contributor Author

jskeet commented Nov 14, 2022

@agoodwin:

Having said that, I couldn't see any reason why Types would have caused a naming collision

Types is always used as a "holder" for nested messages and enums. For example, consider:

message Outer {
  message Nested {
  }
}

That generates (modulo partial etc):

public class Outer
{
    public class Types
    {
        public class Nested { }
    }
}

And we can't only add the suffix for a field if there happen to be no nested enums/messages at the moment, as otherwise adding a nested enum/message would become a breaking change.

@agoodwin
Copy link

agoodwin commented Nov 14, 2022

Ah, thanks @jskeet, it is something fairly obvious then. I've just never made use of nested types so that's the obvious point I was missing.

And that makes total sense not to conditionally apply the suffix or anything complex like that as well. I shall stick with finding a more specific name than just types in the future, to avoid the suffix, and hopefully this conversation is useful to someone else in the future too.

adellahlou pushed a commit to adellahlou/protobuf that referenced this pull request Apr 20, 2023
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.

None yet

3 participants