Skip to content
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

Replace missing/default with {load,dump}_default #1742

Merged
merged 2 commits into from
Jul 21, 2021

Conversation

sirosen
Copy link
Contributor

@sirosen sirosen commented Feb 9, 2021

This is part of #778 , which has been open but inactive for a while.
I've made aggressive use of warnings here and chose the load_default and dump_default names (of the options proposed) because they are parallel with load_key, dump_only, etc in putting the de/serialization qualifier first.

I expect that if this is released, a lot of users of libraries like apispec would start seeing warnings.
I'm not sure what the best way to mitigate that is, but if removing or changing warnings on some of these behaviors would improve things, just let me know.


Add these new parameters to the Field initializer and to the attributes of all fields. Remove the existing missing/default attributes in lieu of the new values.

If a 'default' or 'missing' value is passed, respect it but emit a warning.

New default and missing field properties wrap access to load_default and dump_default, but also emit warnings on access.

Update all usage in tests to the newer form.

@lafrech
Copy link
Member

lafrech commented Feb 9, 2021

I expect that if this is released, a lot of users of libraries like apispec would start seeing warnings.

We can ship a major using new param names. Or a minor with a condition to pick the appropriate parameter according to marshmallow version (extra work, but a major just to bump marshmallow version is not appealing).

@lafrech
Copy link
Member

lafrech commented Apr 8, 2021

I'm still willing to do this name change.

Question is should we postpone it to v4 or deprecate/warn (this PR) now and remove in v4.

I don't mind either way. Maybe we're overthinking it with the deprecation/warning cycle. OTOH, the job is done already.

We're about to ship webargs 8 so now would be a good time to drop the old names.

I had a quick look at apispec / webargs / flask-smorest and I think it should be easy to support without a breaking change by updating the tests / docstrings and add a few conditions to support the old names (not so many places).

@sirosen
Copy link
Contributor Author

sirosen commented Apr 9, 2021

If a deprecation warning is a big enough deal to potentially hold back this change, I am open to removing the warnings and leaving the deprecation in the docs only.

I'd really prefer to use load_default and dump_default. The default/missing names today are just too easy to mix up.

@lafrech
Copy link
Member

lafrech commented Apr 9, 2021

No, those warnings are good. Better than doc only. The alternative was postpone to v4. But I prefer to merge this now.

@sirosen
Copy link
Contributor Author

sirosen commented Apr 16, 2021

I just rebased this to cleanup the conflict.

After more thought, I think the easiest thing is for downstream projects to do

if MARSHMALLOW_VERSION < (3,12):
    ...
else:
    ...

as necessary if they want to avoid the warnings and be compatible with lower marshmallow 3.x versions.

Copy link
Member

@lafrech lafrech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Just a minor comment. Not really blocking.

src/marshmallow/fields.py Outdated Show resolved Hide resolved
Add these new parameters to the Field initializer and to the
attributes of all fields. Remove the existing `missing/default`
attributes in lieu of the new values.

If a 'default' or 'missing' value is passed, respect it but emit a
warning.

New `default` and `missing` field properties wrap access to
load_default and dump_default, but also emit warnings on access.

Update all usage in tests to the newer form.
Warn whenever `missing` or `default` are passed, even if
`dump_default` or `load_default` were passed as well.
@sirosen sirosen force-pushed the better-default-param-names branch from 6511567 to ddd6a51 Compare May 12, 2021 16:16
@sirosen
Copy link
Contributor Author

sirosen commented May 12, 2021

There was a conflict because dev changed near some of this -- no actual changes needed -- so I've done a rebase.

@lafrech lafrech requested a review from sloria June 28, 2021 08:56
@lafrech
Copy link
Member

lafrech commented Jun 28, 2021

@sloria, may we proceed with this?

Copy link
Member

@sloria sloria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with moving forward with this. @lafrech Do you have time to do the changelog and release?

@lafrech
Copy link
Member

lafrech commented Jul 6, 2021

OK, the first who gets the time does it.

But I'd rather settle on #1843 and ship a 3.12.2 bugfix release before.

@lafrech
Copy link
Member

lafrech commented Jul 21, 2021

I just released apispec 5.0.0b1 with marshmallow 3.13 support. 5.0.0 is almost ready, just minor stuff left, but it leaves a little time for people to test the beta version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants