Skip to content

Convert test usages of field metadata to new style#617

Closed
sirosen wants to merge 1 commit intomarshmallow-code:devfrom
sirosen:metadata-style
Closed

Convert test usages of field metadata to new style#617
sirosen wants to merge 1 commit intomarshmallow-code:devfrom
sirosen:metadata-style

Conversation

@sirosen
Copy link

@sirosen sirosen commented Dec 16, 2020

Rather than passing extra keyword args, use metadata=dict(...)
This makes the tests only compatible with the latest version of marshmallow. (At time of writing, a PR branch, in fact.)


This is really meant as a counterpart to marshmallow-code/marshmallow#1702
It can't merge until/unless that merges and releases. It would make the tests only compatible with very new versions of marshmallow though... So I'm not sure if we would want to do that or not? If we don't do this then the tests would start showing the deprecation warnings.

I didn't find any cases of metadata kwargs to fields being used in docs/ but maybe I missed it.

Rather than passing extra keyword args, use `metadata=dict(...)`
This makes the tests only compatible with the latest version of
marshmallow. (At time of writing, a PR branch, in fact.)
@lafrech
Copy link
Member

lafrech commented Jan 20, 2021

Yeah, technically, bumping marshmallow minimum version, even to a minor version, should happen in a major apispec version, I suppose.

Our options are

  • 1/ Release a major apispec version. This minor change doesn't seem to deserve it.
  • 2/ Use conditional code to pass metadata properly depending on marshmallow version. Not sure it is worth the work.
  • 3/ Catch the deprecation error during the tests to hide it as known issue.
  • 4/ Let it go.

My preference goes to 3 until next major. In next major, require marshmallow 3.10.

Your PR seems to contain other reworks, reformatting. I can't tell which tool did that but those could be applied now.

@sirosen
Copy link
Author

sirosen commented Jan 21, 2021

Your PR seems to contain other reworks, reformatting. I can't tell which tool did that but those could be applied now.

Oh, whoops! I think I forgot to turn off isort and didn't notice!
Those weren't meant to be there; please ignore. 😅

I agree with your call on doing option 3.
I'll do a separate PR to figure out how to catch the deprecation warnings. Let's just close this.

@sirosen sirosen closed this Jan 21, 2021
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.

2 participants