-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Serialize Enum to underscored String by default #10431
Serialize Enum to underscored String by default #10431
Conversation
Co-authored by [email protected]
spec/std/json/serialization_spec.cr
Outdated
|
||
describe "Enum::NumberOrStringConverter" do | ||
it "normal enum" do | ||
converter = Enum::NumberOrStringConverter(JSONSpecEnum) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me it's a bit strange that it's number or string for reading, but just number for writing. Maybe we should just let it work with numbers? I can't imagine why one would like to use sometimes strings and sometimes numbers for reading, but just numbers for writing. It should always be either always numbers or always strings.
If it's because of backwards compatibility, maybe that could be in a shard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the idea was to have a simple converter for an easy migration from the current behaviour. You can just plug in Enum::NumberOrStringConverter
and it works.
I agree that a more strict number converter would probably be better. But we didn't have that until now, so I would see that as an additional feature.
If we accept a reduced backward compatibility, we could leave NumberOrStringConverter
out and add a strict NumberConverter
directly for the main use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the path to be more strict. I though that number or string was introduced as an option in the former PR.
Let's introduce Enum::NumberConverter
and leave the default as proposed to string / List of string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deserialization from number or string has been there since Enum.from_json
was implemented. I wouldn't expect the string variant to be used much, because serialization was only as number.
So I'll change the converter to a strict NumberConverter
.
If there's some actual demand, we could consider publishing NumberOrStringConverter
as a shard or whatever, but it's really trivial to implement. And as I said, I don't expect much use anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed the name to more generalized ValueConverter
. I think that's better for describing the purpose.
The two specs failures are probably caused by libyaml writing the document end marker only after some scalars. We really shouldn't be bothered by this, I'll just add an |
Updating big project, quite hard to write every where converters like: @[JSON::Serializable::Options(enums_as_value: true)]
class A
include JSON::Serializable
@a : MyEnum?
end
|
Instead of using a converter on the serializable field, you could also change the implementation of the enum types themselves. Maybe that would be easier for your use case? I don't think adding a global option to |
This picks up #9905 and introduces a stricter default serialization plus a converter for optionally using the current, number-based serialization.
The new behaviour for
Enum
JSON/YAML serialization:The new type
Enum::NumberOrStringConverter
implements the previous behaviour:This type can be used with
*::Serializable
as a*::Field
converter. It also provides a convenient API to use it directly (this could probably be generalized in a followup).The example also applies to YAML serialization, using the appropriate methods and annotations.
Documentation is currently still missing, I'll add that later.
This PR also introduces two new public helper methods that I found convenient for the implementation:
JSON::PullParser.raise
is the equivalent toYAML::Nodes::Node#raise
and offers a simple interface for raisingJSON::ParseException
at the current locationYAML::Nodes::Node#type
returns the name of the node type which is used in error messages to describe the actual token type (vs. the expected one).Resolves #9905
Resolves #9881
I chose to open a new PR instead of amending the previous one in order to start a fresh review. Most of the discussion was about fleshing out the intended behaviour, which should have better been discussed in #9881.
Thanks to @caspiano for the first implementation 🙏