Adds support to specify integer and bool proto fields as string. #253
Conversation
| @@ -17,7 +17,15 @@ def self.default | |||
| # # | |||
|
|
|||
| def acceptable?(val) | |||
| [true, false].include?(val) | |||
| [true, false].include?(val) || (val.is_a?(String) && %w(true false).include?(val)) | |||
There was a problem hiding this comment.
IMO the class checking is not necessary, you could remove that and everything else would work
|
This is super helpful, thanks! LGTM |
|
Also looks like this fixes #249 |
Also fixes the range check for integer types.
|
Addressed comments. PTAL? cc: @localshred |
|
Can you provide a reason why this use case should exist? I'm not convinced we should support magically coercing strings into bools or ints. |
|
@localshred well one reason would be to fix the inconsistent serialization issue (#249) 😁 Another would be that we often build protobuf messages out of query params, where everything is stringified, and moving this coercion here matches our expectations as customers of this gem. |
| rescue | ||
| false | ||
| end | ||
|
|
||
| def coerce!(val) | ||
| return val.to_i if val.is_a?(Numeric) |
There was a problem hiding this comment.
Is this required? (the .to_i and is_a?(Numeric) check)
we have been looking at standardizing serialization with this branch
https://github.com/localshred/protobuf/compare/bdewitt/fix_coercion
There was a problem hiding this comment.
It is required to convert a float to an integer, unless that should be disallowed.
Also, Integer('0x10') produces 16. IMO, it should return an error and hence requires Integer(val, 10) which throws an error if val is of type Numeric.
|
Ping! |
|
Pinging again! Please let me know if there are any additional changes to address. |
|
Sorry @hrsht, was out of town all last week. Will look into this some more this week. |
Adds support to specify integer and bool proto fields as string.
Also fixes the range check for integer types.
r: @zachmargolis