Make field mask inner public #465
Conversation
There was a problem hiding this comment.
It's probably easiest to allow users to just let users create such masks directly, after all they serialize into comma-separated lists like "a,b"
However, it turns out that these must be snake-case, and anything the user passes in should adhere to that.
Thus I recommend adding a constructor instead that converts to snake-case, just like the deserialization (FromStr) does.
|
Okay sounds good. I made the change, and tested it locally although I was already using a non-snake-case string That said, as you mentioned the specification isn't exactly simple. I think in order to use the field correctly in the first place users will have to read up on its use, and if despite doing that they're passing the wrong values in it might be best to allow the api call to fail to let them know they need to re-read. Magically lowercasing but not checking field existence, etc might be more complex implicit behavior. |
|
Or were you thinking it should just be |
There was a problem hiding this comment.
Thanks!
The type already exhibits this behaviour and it internally wants to have snake-case - the constructor enforces this invariant and that seems fine to me.
Nothing changes otherwise, the user would always have to know valid strings for field names and there is no type-safety for that (or it isn't implemented).
Or were you thinking it should just be
FieldMask(pub String)? I wouldn't mind that, it'd convey to users that they're completely on their own which I think might be good.
No, let's just keep it as it is and not overthink this type.
The inner value was private so it couldn't be constructed directly (without deserializing from a string indirectly). Creating it directly seems safer than deserializing, especially since I'm unfamiliar with the format.
Was it intended for this to be constructed directly or is there another mechanism for setting these up? I thought the api might do it directly but it doesn't atm afaict.