qitab / cl-protobufs

Common Lisp protocol buffer implementation.
MIT License
84 stars 18 forks source link

Replace 'optional' with 'proto2-optional' and 'singular' with 'proto3-optional'. #180

Closed bkuehnert closed 4 years ago

bkuehnert commented 4 years ago

The term 'singular' could be used to refer to non-repeated fields. These names make it clear which semantics are in use for a particular optional field.

Slids commented 4 years ago

You have branch conflicts. Also I don't like this change. I like the :singular field as it conforms to the documentation, and the word optional is annoying and overloaded... Also it's weird to see proto2-optional in proto3.

bkuehnert commented 4 years ago

You have branch conflicts. Also I don't like this change. I like the :singular field as it conforms to the documentation, and the word optional is annoying and overloaded... Also it's weird to see proto2-optional in proto3.

Posting this here as well: https://developers.google.com/protocol-buffers/docs/reference/cpp-generated Sadly, it doesn't follow the documentation. The word "singular" is used as a synonym for "non-repeated".

Then again, this is documentation, not code. As for actual code: The C++ code handles this problem by having proto2 and proto3 singular fields share the label "optional". This is done for backwards compatibility reasons, which we don't have to worry about (?). For us, we can use separate labels for proto2 optional and proto3 optional fields. The only way to differentiate between these two is just proto2 vs proto3...

cgay commented 4 years ago

I was looking at our descriptor class hierarchy (or lack thereof) yesterday and thought of a different way to handle this. The base descriptor class could have the syntax slot, set to :proto2 or :proto3, hence all {file,field,message}-descriptor objects have easy access to the syntax. Then we can continue to just use :optional as the field label but check the syntax when a distinction is necessary. WDYT?

bkuehnert commented 4 years ago

I was looking at our descriptor class hierarchy (or lack thereof) yesterday and thought of a different way to handle this. The base descriptor class could have the syntax slot, set to :proto2 or :proto3, hence all {file,field,message}-descriptor objects have easy access to the syntax. Then we can continue to just use :optional as the field label but check the syntax when a distinction is necessary. WDYT?

Actually, we don't need to make that distinction at runtime at all. We only need to distinguish between proto2 & proto3 semantics at compile time (for creating the accessors and has-* functions). After that, both field types use the same interface for (de)serialization/text-format/json, thanks to Jon's changes.

So, I suggest that we use :optional for both, and don't even add a syntax slot.

cgay commented 4 years ago

SGTM

bkuehnert commented 4 years ago

Closing this PR. I'll make a new one with the other change.