Support hashes for struct initializers #5716

jbolinger · 2019-02-12T22:50:49Z

This change simplifies working with Google::Protobuf::Struct and Google::Protobuf::Value so that code like this:

Google::Protobuf::Struct.new(fields: {
  'a' => Google::Protobuf::Value.new({number_value: 2.0})
})

can be written as:

Google::Protobuf::Struct.new(fields: {
   'a' => {number_value: 2.0}
})

This would simplify struct usage in many practical cases, such as: googleapis/google-cloud-ruby#2839

I believe this also resolves #3120.

ruby/tests/well_known_types_test.rb

blowmage

👍

TeBoring · 2019-02-24T18:23:43Z

ruby/ext/google/protobuf_c/map.c

@@ -71,6 +71,9 @@ static VALUE table_key(Map* self, VALUE key,
    case UPB_TYPE_BYTES:
    case UPB_TYPE_STRING:
      // Strings: use string content directly.
+      if (TYPE(key) == T_SYMBOL) {


Does this also changes API of normal map fields?

I don't think so, and it didn't break any tests that I'm aware of. Can you think of a case where it would make a difference?

This would not throw type errors in some cases where it would have previously but I don't know if that should be considered additive vs a breaking change?

It doesn't break existing test cases. However, it allows symbol for string map, which is not allowed previously.

Right, is there any reason to prohibit that?

It makes map not consistent with optional, repeated fields.
Besides, how to compare symbol? identity compare? value compare? Is the semantic same as string?
Besides, is the feature quite important? Otherwise, we don't want to over-provide it, as these trivial features necessarily makes our library complicate.

It also breaks existing type checking. We should added tests for our type checking.

It allows users to provide:

Google::Protobuf::Struct.new(fields: { a: {number_value: 2.0} })

instead of:

Google::Protobuf::Struct.new(fields: { 'a' => {number_value: 2.0} })

I would argue that Converting Symbol key values to String in Struct is a beneficial affordance, as Struct will only every have String keys. Map can have any any value type as a key, so it wouldn't make as much sense.

That said, I do think it would be fine to accept Symbol values every time a String is expected. In Ruby, I would write it like this:

class Google::Protobuf::Struct def [](key) inner_struct[String(key)] end end

Basically, anytime we expect a String object, pass it to the String() method to perform the necessary conversions.

jbolinger · 2019-02-28T22:21:30Z

@TeBoring what kind of type checking tests did you have in mind? I'm happy to add them but I'm not sure exactly what you're asking for.

TeBoring · 2019-03-02T04:42:11Z

The check test I have in mind is something like:

// expect throw error
a.foo = 1  // foo is a string field

However, this could be a separate change.

TeBoring · 2019-03-02T04:44:20Z

If ruby users do expect Symbol to be the same as string, I think we should make it consistent in all our APIs.
However, we need a API review to allow that. @jbolinger Could you write a doc for that?
My suggestion is allowing Symbol could be another change. We don't need to do it in this change.

jbolinger · 2019-03-06T00:19:17Z

I'm not sure that the inconsistency is all that bad. Backing up a bit, the first version of this PR didn't include the symbol bit. I was trying to fix an existing issue that prevented using a hash to init a map value:

So, instead of this:

Google::Protobuf::Struct.new(fields: {
  'a' => Google::Protobuf::Value.new({number_value: 2.0})
})

one could write a more concise version:

Google::Protobuf::Struct.new(fields: {
   'a' => {number_value: 2.0}
})

I shouldn't speak for @blowmage, but he pointed out that getting rid of the hash rocket would look even better. It's shorter and likely the more commonly used syntax. So, I added the implicit string/symbol conversion to allow this:

Google::Protobuf::Struct.new(fields: {
   a: {number_value: 2.0}
})

Sure, it's a little inconsistent but since a map key can't be symbol allowing either to be used in this context seems like a useful convenience. I'm not sure if I understand what you mean by making it consistent for optional fields or treating string/symbol as the same, etc. ~~but I don't think it makes sense to support something like this~~:

msg.a_string = :some_thing

~~That should probably require an explicit conversion and, even if we did support it, I'm not sure it would actually be useful in very many situations.~~

Never mind, that actually works fine already. What else were you thinking we should change?

jbolinger · 2019-03-26T17:56:55Z

@TeBoring any updates on this (and could you run the tests again)?

TeBoring · 2019-03-26T23:52:09Z

I mean please make the Symbol a separate change.

jbolinger · 2019-03-27T00:42:57Z

@TeBoring what other use cases / changes did you have in mind? It looks like Symbol behaves as a string in certain contexts already (see my previous example). What else should we support?

TeBoring · 2019-03-27T01:00:44Z

I see your comment. So you only want to make string map key to accept symbol?

…

On Tue, Mar 26, 2019 at 17:43 Joe Bolinger ***@***.***> wrote: @TeBoring <https://github.com/TeBoring> what other use cases / changes did you have in mind? It looks like Symbol behaves as a string in certain contexts already (see my previous example). What else should we support? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5716 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE9H5bP6pI_dDmKy6JUliMS8IUqeccFLks5var6kgaJpZM4a4N8P> .

TeBoring · 2019-03-27T01:02:46Z

Only for the map in struct?

…

On Tue, Mar 26, 2019 at 18:00 Paul Yang ***@***.***> wrote: I see your comment. So you only want to make string map key to accept symbol? On Tue, Mar 26, 2019 at 17:43 Joe Bolinger ***@***.***> wrote: > @TeBoring <https://github.com/TeBoring> what other use cases / changes > did you have in mind? It looks like Symbol behaves as a string in certain > contexts already (see my previous example). What else should we support? > > — > You are receiving this because you were mentioned. > > > Reply to this email directly, view it on GitHub > <#5716 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AE9H5bP6pI_dDmKy6JUliMS8IUqeccFLks5var6kgaJpZM4a4N8P> > . >

TeBoring · 2019-03-27T01:07:39Z

If you could make the change only affect the map in struct but not other maps, I am fine with that.

…

On Tue, Mar 26, 2019 at 18:02 Paul Yang ***@***.***> wrote: Only for the map in struct? On Tue, Mar 26, 2019 at 18:00 Paul Yang ***@***.***> wrote: > I see your comment. So you only want to make string map key to accept > symbol? > On Tue, Mar 26, 2019 at 17:43 Joe Bolinger ***@***.***> > wrote: > >> @TeBoring <https://github.com/TeBoring> what other use cases / changes >> did you have in mind? It looks like Symbol behaves as a string in certain >> contexts already (see my previous example). What else should we support? >> >> — >> You are receiving this because you were mentioned. >> >> >> Reply to this email directly, view it on GitHub >> <#5716 (comment)>, >> or mute the thread >> <https://github.com/notifications/unsubscribe-auth/AE9H5bP6pI_dDmKy6JUliMS8IUqeccFLks5var6kgaJpZM4a4N8P> >> . >> >

TeBoring · 2019-03-27T01:31:16Z

Never mind. I think I can accept this change. I want to let @haberman to also take a look, since this affects API.

jbolinger · 2019-03-27T16:42:47Z

Thanks @TeBoring! I see a build failure but I don't think it's related to this, but let me know if it is or if anything else needs to be done.

jbolinger · 2019-07-24T22:22:42Z

Are you still interested in accepting this change? I think it's a nice simplification but it fell off my radar. Let me know if I can do anything else.

TeBoring · 2019-07-24T22:56:08Z

It seems ruby 2.3 still failed?

jbolinger · 2019-07-24T23:08:52Z

Hmm, that's sort of a strange error that looks like it might be related to the rvm or something unrelated in the environment (openssl?). I can try to reproduce it when I'm back in the office on Monday.

TeBoring · 2019-07-24T23:28:19Z

Maybe sync and push?

TeBoring · 2019-07-25T18:16:16Z

https://www.ruby-lang.org/en/news/2019/03/31/support-of-ruby-2-3-has-ended/

Since the support for ruby 2.3 has been ended, we will deprecate ruby 2.3.

support hashes for struct initalizers

41e9d3b

googlebot added the cla: yes label Feb 12, 2019

blowmage reviewed Feb 12, 2019

View reviewed changes

ruby/tests/well_known_types_test.rb Show resolved Hide resolved

blowmage reviewed Feb 12, 2019

View reviewed changes

ruby/tests/well_known_types_test.rb Show resolved Hide resolved

jbolinger added 2 commits February 12, 2019 18:26

convert hash keys to string

1c97d30

update tests

d60d41e

TeBoring self-assigned this Feb 14, 2019

TeBoring added kokoro:run ruby labels Feb 14, 2019

kokoro-team removed the kokoro:run label Feb 14, 2019

blowmage suggested changes Feb 14, 2019

View reviewed changes

ruby/tests/well_known_types_test.rb Show resolved Hide resolved

add extra asserts

0d374e9

blowmage approved these changes Feb 14, 2019

View reviewed changes

TeBoring reviewed Feb 24, 2019

View reviewed changes

TeBoring added the kokoro:run label Feb 24, 2019

kokoro-team removed kokoro:run labels Feb 24, 2019

TeBoring added the kokoro:run label Feb 28, 2019

kokoro-team removed the kokoro:run label Feb 28, 2019

TeBoring added the kokoro:run label Mar 26, 2019

kokoro-team removed the kokoro:run label Mar 26, 2019

TeBoring added the kokoro:force-run label Mar 27, 2019

kokoro-team removed the kokoro:force-run label Mar 27, 2019

TeBoring added kokoro:run labels Jul 24, 2019

protobuf-kokoro removed the kokoro:run label Jul 24, 2019

TeBoring approved these changes Jul 25, 2019

View reviewed changes

TeBoring merged commit 41e1234 into protocolbuffers:master Jul 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support hashes for struct initializers #5716

Support hashes for struct initializers #5716

jbolinger commented Feb 12, 2019

blowmage left a comment

TeBoring Feb 24, 2019 •

edited

jbolinger Feb 25, 2019

TeBoring Feb 28, 2019

jbolinger Feb 28, 2019

TeBoring Feb 28, 2019

TeBoring Feb 28, 2019

blowmage Feb 28, 2019

jbolinger commented Feb 28, 2019

TeBoring commented Mar 2, 2019

TeBoring commented Mar 2, 2019

jbolinger commented Mar 6, 2019 •

edited

jbolinger commented Mar 26, 2019

TeBoring commented Mar 26, 2019

jbolinger commented Mar 27, 2019

TeBoring commented Mar 27, 2019 via email

TeBoring commented Mar 27, 2019 via email

TeBoring commented Mar 27, 2019 via email

TeBoring commented Mar 27, 2019

jbolinger commented Mar 27, 2019

jbolinger commented Jul 24, 2019

TeBoring commented Jul 24, 2019

jbolinger commented Jul 24, 2019

TeBoring commented Jul 24, 2019

TeBoring commented Jul 25, 2019

Support hashes for struct initializers #5716

Support hashes for struct initializers #5716

Conversation

jbolinger commented Feb 12, 2019

blowmage left a comment

Choose a reason for hiding this comment

TeBoring Feb 24, 2019 • edited

Choose a reason for hiding this comment

jbolinger Feb 25, 2019

Choose a reason for hiding this comment

TeBoring Feb 28, 2019

Choose a reason for hiding this comment

jbolinger Feb 28, 2019

Choose a reason for hiding this comment

TeBoring Feb 28, 2019

Choose a reason for hiding this comment

TeBoring Feb 28, 2019

Choose a reason for hiding this comment

blowmage Feb 28, 2019

Choose a reason for hiding this comment

jbolinger commented Feb 28, 2019

TeBoring commented Mar 2, 2019

TeBoring commented Mar 2, 2019

jbolinger commented Mar 6, 2019 • edited

jbolinger commented Mar 26, 2019

TeBoring commented Mar 26, 2019

jbolinger commented Mar 27, 2019

TeBoring commented Mar 27, 2019 via email

TeBoring commented Mar 27, 2019 via email

TeBoring commented Mar 27, 2019 via email

TeBoring commented Mar 27, 2019

jbolinger commented Mar 27, 2019

jbolinger commented Jul 24, 2019

TeBoring commented Jul 24, 2019

jbolinger commented Jul 24, 2019

TeBoring commented Jul 24, 2019

TeBoring commented Jul 25, 2019

TeBoring Feb 24, 2019 •

edited

jbolinger commented Mar 6, 2019 •

edited