search

NowanIlfideme / pydantic-yaml

YAML support for Pydantic models
MIT License
140 stars 11 forks source link

Possibly change to mixin? #1

Closed NowanIlfideme closed 2 years ago

NowanIlfideme commented 3 years ago

Instead of subclassing BaseModel, perhaps YamlModel can work as a mixin? That way you could use it alongside BaseSettings, for example, or other custom class hierarchies. It might already work, but I haven't had time to test extensively. 😄

mvoitko commented 3 years ago

It would be very nice also for advanced case like using Generic models.

NowanIlfideme commented 3 years ago

Hmm. So, I remember what my test implementation looked like. The main pain point is this enum in Pydantic: https://github.com/samuelcolvin/pydantic/blob/master/pydantic/parse.py#L10 If there was a way to just define an additional protocol, that Pydantic would hook into, then we might not even need a separate class for parsing YAML into Pydantic. For converting your model to YAML, a mixin would work well - we could even supply multiple mixins, depending on what library you're using.

However, the main dev does not want any PRs for parse.py for various reasons (see https://github.com/samuelcolvin/pydantic/issues/136#issuecomment-370232244).

NowanIlfideme commented 3 years ago

Wrote up https://github.com/samuelcolvin/pydantic/discussions/3025 - if accepted, this would mean pydantic_yaml would be able to support YAML for arbitrary models, while allowing dumping via specialized mixins.

mvoitko commented 3 years ago

But could we in the meantime change pydantic_yaml to mixins now?

NowanIlfideme commented 3 years ago

I think we could, but because it's a breaking change, I'd prefer to plan through, to avoid more breaking changes in the future. Some more thoughts:

  1. (+) More elegant than current approach.
  2. (+) Can implement model.yaml() method in separate classes for pyyaml and ruamel.yaml, and import whichever library you have installed.
  3. (-) Breaking change (unavoidable). a. (+) Of course, we could just add a separate mixin class, and have YamlModel just inherit from the mixin.
  4. (-) Need to change how loading is done, because parse_raw() might get overwritten by another class. a. (-) This means we probably need to monkey-patch pydantic.parse rather than using a class method. b. ( ) Alternatively, users need to be vigilant to their inheritance order when using the mixin.

@mvoitko if you suggest a PR for this, I'll be happy to look over it. Otherwise I'll try to do it myself later this week. 😃

NowanIlfideme commented 2 years ago

@mvoitko I've implemented this in 0.5.0, please take a look, if it's still relevant for you. 😉