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

Support hashes for struct initializers #5716

Merged
merged 4 commits into from
Jul 25, 2019

Conversation

jbolinger
Copy link
Contributor

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.

Copy link
Contributor

@blowmage blowmage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -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) {
Copy link
Contributor

@TeBoring TeBoring Feb 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this also changes API of normal map fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, is there any reason to prohibit that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

@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
Copy link
Contributor

TeBoring commented Mar 2, 2019

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
Copy link
Contributor

TeBoring commented Mar 2, 2019

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
Copy link
Contributor Author

jbolinger commented Mar 6, 2019

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
Copy link
Contributor Author

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

@TeBoring
Copy link
Contributor

I mean please make the Symbol a separate change.

@jbolinger
Copy link
Contributor Author

@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
Copy link
Contributor

TeBoring commented Mar 27, 2019 via email

@TeBoring
Copy link
Contributor

TeBoring commented Mar 27, 2019 via email

@TeBoring
Copy link
Contributor

TeBoring commented Mar 27, 2019 via email

@TeBoring
Copy link
Contributor

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

@jbolinger
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

It seems ruby 2.3 still failed?

@jbolinger
Copy link
Contributor Author

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
Copy link
Contributor

Maybe sync and push?

@TeBoring
Copy link
Contributor

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.

@TeBoring TeBoring merged commit 41e1234 into protocolbuffers:master Jul 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ruby: Allow for the instantiation of nested message fields with ruby-hashes
6 participants