Marshmallow: RFC: Replace missing/default with default_load/default_dump/default

Created on 19 Apr 2018  ·  10Comments  ·  Source: marshmallow-code/marshmallow

Quoting @deckar01 in https://github.com/marshmallow-code/marshmallow/issues/775#issuecomment-382130144:

It does seem arbitrary for default to only be used for serialization. It isn't immediately obvious that missing is the deserialization complement. It would be nice if there was a little more symmetry here. Maybe something like default_load, default_dump, and default to set both at once.

backwards incompat feedback welcome

Most helpful comment

So should it be default_load and default_dump only?

If we do that we can keep and deprecate the old names:

  • missing (deprecated) -> load_default
  • default (deprecated) -> dump_default

(I prefer load_default to default_load.)

All 10 comments

I've definitely gotten tripped up by this when I started using Marshmallow.

However, I'm not sure making default do both is less surprising. I don't think I've ever _wanted_ to set both, and it seems like it'd be error-prone for the "obvious" flag to set a dump-time default, which is generally a fairly rare occurrence when dumping e.g. objects.

So should it be default_load and default_dump only?

If we do that we can keep and deprecate the old names:

  • missing (deprecated) -> load_default
  • default (deprecated) -> dump_default

(I prefer load_default to default_load.)

I'm lukewarm about this, only because I'm hesitant to add any more drastic backwards-incompatible changes to 3.0. Sure, the difference between default and missing isn't immediately obvious, but it's one of those things you learn just once.

tl;dr I'm open to something like load_default and dump_default, but I don't think it's worth delaying the 3.0 final release.

I agree. I'm eager to see marshmallow 3 out.

I just sent a PR with the change. If this makes it to marshmallow 3, fine. Otherwise, no problem.

I just realized I used default_dump while I preferred dump_default. Well done...

I can fix that with a find&replace. @sloria, @taion, @deckar01 feel free to suggest better names before I do so.

I think dump_default and load_default are good names. This default/missing distinction is something that every developer here, myself included, has gotten tripped up over. That said, emitting a FutureWarning and making this a deprecation is not much worse than what's here.

IMHO, there are so may changes every app will be broken when going from MA2 to MA3, so I don't mind hard changes (as opposed to deprecation warnings) if the fix is obvious (more or less find&replace). I don't know the good practices.

Aside from breaking users' code, I'm also concerned about the maintenance burden for libraries that support both marshmallow 2 and 3. webargs, apispec, and environs will probably be affected by this change. I'm sure there are others.

That's fair.

I just checked, the changes in webargs and apispec are minimal, very localized. I could submit the PRs.

But I understand the cost/benefit tradeoff and the concern about other libs.

I don't mind either way.

I have seen other libraries use a mono repo with synchronized releases to avoid the maintenance burden of cross supporting major versions. That also makes it a lot easier to detect breaking changes before they get released, because the CI test suites run together.

It might not hurt to adopt a more strict deprecation policy. For naming changes like this we could release it in any minor version of v3 and schedule the current names for depreciation later.

It might not hurt to adopt a more strict deprecation policy. For naming changes like this we could release it in any minor version of v3 and schedule the current names for depreciation later.

We could also release the new names in 3.0.0 and only raise a PendingDeprecationWarning about the old ones. Then raise a DeprecationWarning in a later 3.x. Then remove in 4.0.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

imhoffd picture imhoffd  ·  3Comments

sloria picture sloria  ·  3Comments

rastikerdar picture rastikerdar  ·  3Comments

k0nsta picture k0nsta  ·  4Comments

zohuchneg picture zohuchneg  ·  3Comments