Namespace extension fields

[nerdrew]

This PR is branched off #302, so the first commit can be ignored.

cc @embark

[zanker]

I'm confused by what this is for, same with the one below

[zachmargolis]

it's for extension names with a dot in the middle -- wouldn't be valid ruby syntax with :foo.bar so it has to be :"foo.bar"

[embark]

You can't do :.string without ruby complaining, you have to do :".string". Now that we are namespacing options and extensions in general, the names will include .

[embark]

Dang it @zachmargolis beat me while I had typed out my answer but failed to press send for 20 minutes :P

[zanker]

My suggestion would be to explicitly use double quotes rather than relying on inspect since it's not totally obvious. Or maybe a comment to clarify?

Sent from my iPhone

On Feb 25, 2016, at 12:44, Zach Margolis notifications@github.com wrote:

In lib/protobuf/generators/field_generator.rb:

@@ -20,7 +20,7 @@ class FieldGenerator < Base
attr_reader :field_options

   def applicable_options

•    @applicable_options ||= field_options.map { |k, v| ":#{k} => #{v}" }
  

  it's for extension names with a dot in the middle -- wouldn't be valid ruby syntax with :foo.bar so it has to be :"foo.bar"

—
Reply to this email directly or view it on GitHub.

[nerdrew]

So:

@applicable_options ||= field_options.map { |k, v| ":\"#{k}\" => #{v}" }

?

Examples:

[1] pry(main)> sym = ".boom.foo".to_sym
=> :".boom.foo"
[2] pry(main)> puts sym.inspect
:".boom.foo"
=> nil
[3] pry(main)> puts %[:"#{sym}"]
:".boom.foo"
=> nil

[nerdrew]

Addressed with a comment.

(Not applicable to proto field names, but inspect is nice: :'boom"'.inspect #=> ":\"boom\\\"\"")

[zanker]

Seems fine, confused by inspect though

[zachmargolis]

So excited for this! Should we do a bigger version bump than a patch since it may be not bavvkwards compat?

[embark]

Agreed that it is not currently backwards compatible in this case:

Before:

> ::Example::Message.new[:extension_field]
=> 2.0

After:

> ::Example::Message.new[:extension_field]
=> ArgumentError: invalid field name=:extension_field
> ::Example::Message.new[:".example.Namespaced.extension_field"]
=> 2.0

[nerdrew]

@embark True. It is not backward compatible. To make it backward compatible, we can store the unnamespaced extension name in the message field store (as both a string and a symbol), but it is getting a little crazy in there.

The field_store would then have the following keys that all refer to the name field: tag, field_name (a symbol), field_name.to_s, unnamespaced field_name, and unnamespaced field_name.to_s.

field_store[1] == field_store[:extension_field] == field_store["extension_field"] == field_store[:".my_package.extension_field"] == field_store[".my_package.extension_field"]

No judgements, but, hmmmm.... yes judgements.

[embark]

@zachmargolis or @film42 thoughts on keeping it backwards compatible now with deprecated accessors, or just doing a bigger version bump? It will be so nice to support extensions with the same name in for the same message.

[zachmargolis]

@embark I think that as good software practices, it would be good make a small bump with a deprecation warning and then a big bump with the breaking change? That is sort of what was outlined in in #196: #196 (comment)

[liveh2o]

It might be ugly, but a clean upgrade path is how we typically try to roll. We might be approaching time for a major version bump where we can cleanup a lot of the "cruft" that we've accumulated.

[embark]

Hey guys so we updated the branch to be backwards compatible. The specs of the new commit show the new accessors that we allow, basically:
field_store[1] == field_store[:extension_field] == field_store["extension_field"] == field_store[:".my_package.extension_field"] == field_store[".my_package.extension_field"] works

[zachmargolis]

yayyyyyy LGTM 👍 😎 🎉

[liveh2o]

One last look, @film42 or @mmmries?

[embark]

@film42 @mmmries beginning of the week ping :)

[film42]

🎉 🎉 🎉 🎉 🎉