marshmallow icon indicating copy to clipboard operation
marshmallow copied to clipboard

Add Enum field

Open lafrech opened this issue 3 years ago • 2 comments

Replaces @orenc17's #1885.

This is a long standing feature request. See for instance discussions in #267.

The status quo is that people can use @justanr's marshmallow_enum.

Currently, this lib doesn't seem actively maintained. I don't blame the author. A few things are outdated, deprecated stuff. The changes since MA3 have been user contributed and some are still missing.

I'm thinking if we recommend this lib as the reference choice, why not integrate it in code? This is what I've been trying to do.

I started by importing the code and updating it for PY3/MA3.

I also removed the dump/load asymmetry (dump by value, load by name) that I found awkward and revamped the error message generation.

I was still not happy with the by_value/by_name mechanism. I serialize by name, so I could live with only this, but I understand the need for serializing by value (to get an int or a nicer string). However, I like the type to be well-defined so that we can inherit deserialization from basic fields, and to help documenting the type (e.g. in apispec).

My proposal is therefore an Enum field serializing by name, and typed enum fields StringEnum and IntegerEnum, to serialize by value with a given type. Users would be free to create new types but I believe those cover most cases.

Transition from marshmallow_enum should be relatively smooth for users who don't use the dump/load asymmetry and who don't rely on exact error messages.

This still lacks a bit of doc but I'd rather get feedback before polishing.

Note we can't use OneOf validator because validation occurs after deserialization and wrong values are caught at deserialization already.

lafrech avatar Jul 20 '22 14:07 lafrech

Funnily enough, I showed up here because I got notified I'm a maintainer of a critical package, which turns out is the one I question.

justanr avatar Jul 21 '22 01:07 justanr

Renamed Enum -> EnumSymbol, TypedEnum -> EnumValue. I'll squash later on.

Docstrings still missing.

lafrech avatar Jul 22 '22 06:07 lafrech

What about EnumValue(String) and EnumValue(Integer)? That is consistent with Nested/List API, improves readability, and minimizes the API surface area.

Just tried this and it worked a treat!

I'm happy with this implementation.

Still requires docstrings and perhaps testing with a DateTime field or some other field for which ser/deser is less trivial to assert ser/deser actually happens.

lafrech avatar Aug 18 '22 10:08 lafrech

I think I'm done.

@sloria @deckar01 @justanr I'll wait for a few days in case someone wants to review this and then I'll merge.

lafrech avatar Aug 19 '22 07:08 lafrech

PR fixed. I'm not 100% happy with self.field._serialize(m.value, None, None) but that's our API.

lafrech avatar Aug 25 '22 15:08 lafrech

Alright, moving on with this.

lafrech avatar Sep 04 '22 15:09 lafrech

Just after merging this, as I was updating the changelog to release, I realized the naming didn't follow our habits of naming fields by object type.

I pushed a new PR to propose merge of both fields into an Enum field: #2044.

lafrech avatar Sep 04 '22 16:09 lafrech

Would you consider adding a short migration guide for marshmallow_enum users? :)

ThiefMaster avatar Sep 29 '22 09:09 ThiefMaster