-
-
Notifications
You must be signed in to change notification settings - Fork 620
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
Conversation
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
Yes. As long as we update with the extra kwargs. We don't want to modify the input dict. |
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.
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? |
Sorry for the delayed response--yes, I think this is good to go into 3.x.
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 |
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. |
I've opened marshmallow-code/apispec#617 as a counterpart to this, but other than that I don't plan to do more. |
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:
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.)dict(metadata)
the right thing? Should we maybe do nothing with the input?metadata
nowmarshmallow
marshmallow-code
projects to updatemetadata
style in docs when this merges?