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

Deprecate metadata as additional field kwargs, support explicit metadata=... #1702

Merged
merged 3 commits into from
Dec 19, 2020

Conversation

sirosen
Copy link
Contributor

@sirosen sirosen commented Dec 2, 2020

I'm opening this to kickstart discussion (and volunteer to do any necessary work). I haven't rewritten any tests or added new tests or updated docs yet.

Add metadata=... as an explicit keyword argument, which dict-ifies a mapping, and treat any additional kwargs as "added metadata" which triggers a deprecation warning.

This is backwards compatible except in the case where someone is already using metadata as the name of a metadata field. Which is probably never intentional if it's done at all.

resolves #1350


Questions/TODOs on this:

  • Is it okay to add metadata=... to kwargs in 3.x or does this whole change have to wait for 4.x ? (I don't think so, maybe others disagree.)
  • Is doing the shallow-copy with dict(metadata) the right thing? Should we maybe do nothing with the input?
  • Rewrite tests to use explicit metadata now
  • Update docs within marshmallow
  • Should I prepare PRs on apispec and other marshmallow-code projects to update metadata style in docs when this merges?

Add `metadata=...` as an explicit keyword argument, which dict-ifies a
mapping, and treat any additional kwargs as "added metadata" which
triggers a deprecation warning.

This is backwards compatible *except* in the case where someone is
already using `metadata` as the name of a metadata field. Which is
probably never intentional if it's done at all.

resolves marshmallow-code#1350
@lafrech
Copy link
Member

lafrech commented Dec 2, 2020

Is doing the shallow-copy with dict(metadata) the right thing? Should we maybe do nothing with the input?

Yes. As long as we update with the extra kwargs. We don't want to modify the input dict.

src/marshmallow/fields.py Outdated Show resolved Hide resolved
sirosen and others added 2 commits December 3, 2020 11:29
Rather than explicitly dict-ifying the input and conditionally
running `update()`, combine data with dict expansion.

Co-authored-by: Steven Loria <sloria1@gmail.com>
There is one test of field metadata which changed to use
`metadata={...}` and a single new test which ensures that you can use
either style for setting field metadata, but that if you use the
"arbitrary keyword arguments" pathway, a deprecation warning will be
emitted.
@sirosen sirosen marked this pull request as ready for review December 11, 2020 18:46
@sirosen
Copy link
Contributor Author

sirosen commented Dec 11, 2020

I just made the testsuite updates (which are quite modest) for this and looked for any narrative docs which should be updated but didn't find any.

The only question I have left -- since nobody has raised a big concern about doing this in 3.x -- is whether or not I should work on doc updates for apispec and other projects?

@sloria
Copy link
Member

sloria commented Dec 11, 2020

Sorry for the delayed response--yes, I think this is good to go into 3.x.

I should work on doc updates for apispec and other projects

Good idea; it'd be good to prep for this in webargs and apispec (I think those are the only 2 marshmallow-code projects that use metadata as part of their API?).

@sirosen
Copy link
Contributor Author

sirosen commented Dec 11, 2020

Okay, awesome, I'll get started on apispec.

I don't think there's anything in webargs to change anymore, but I'll double-check.

@sirosen
Copy link
Contributor Author

sirosen commented Dec 16, 2020

I've opened marshmallow-code/apispec#617 as a counterpart to this, but other than that I don't plan to do more.
So as far as I'm concerned, this is ready. Just let me know if there's more I should do. 🙂

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.

RFC: Change the way we store metadata?
3 participants