Skip to content

Conversation

@whatyouhide
Copy link
Collaborator

cc @keathley as mentioned in #76 and cc @tony612 for review 🙃

@whatyouhide
Copy link
Collaborator Author

Ping @tony612 :)

@tony612
Copy link
Collaborator

tony612 commented Dec 19, 2019

@whatyouhide Thanks for the PR. I'm thinking if it's better to have a new! function and keep the original one. The reason is sometimes I found it's useful to use a superset map to create a pb struct. And also, in this way we can avoid breaking current code. How do you like it? @keathley

@whatyouhide
Copy link
Collaborator Author

@tony612 we use this library pretty extensively at work and in my experience this is definitely an extremely unexpected behaviour, so much so that I've always considered this a bug. I think it's much more common to misspell a field name and only catch it when encoding than to desire this behaviour. Ultimately, up to you, I can create new! :)

@tony612
Copy link
Collaborator

tony612 commented Dec 20, 2019

@whatyouhide I guess that's why we have both struct and struct!? Sometimes we specify the keys explicitly

Foo.new(a: 1, b: "foo")

But sometimes we use a map to build it like

map = got_from_somewhere_else()
Foo.new(map)

It's hard to guarantee the map in the later example match very well with our struct, especially in a dynamic language like Elixir.

@whatyouhide
Copy link
Collaborator Author

@tony612 okay, we'll switch completely to new! and forget about new then 😄 I updated the PR, can you take a look?

@tony612
Copy link
Collaborator

tony612 commented Dec 23, 2019

@whatyouhide Looks like new! should be used in the test?

@whatyouhide
Copy link
Collaborator Author

whatyouhide commented Dec 23, 2019

@tony612 Yep, I actually forgot to push this code but we need to generate the new!/1 function in protobuf modules too 😄 I just pushed the new code and the updated test!

@tony612 tony612 merged commit 70caf73 into elixir-protobuf:master Dec 25, 2019
@whatyouhide whatyouhide deleted the check-struct-fields branch December 25, 2019 09:40
@keathley
Copy link

Sorry I didn't respond earlier. I'm glad y'all ended up using new! since it allows new to remain "open" which should allow more re-use of existing maps. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants