Skip to content

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

Merged
sloria merged 3 commits intomarshmallow-code:devfrom
sirosen:allow-metadata-fieldarg
Dec 19, 2020
Merged

Deprecate metadata as additional field kwargs, support explicit metadata=...#1702
sloria merged 3 commits intomarshmallow-code:devfrom
sirosen:allow-metadata-fieldarg

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.

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