Skip to content

Support passing deprecated in the metadata dict#686

Merged
lafrech merged 2 commits intomarshmallow-code:devfrom
greyli:add-deprecated
Jun 28, 2021
Merged

Support passing deprecated in the metadata dict#686
lafrech merged 2 commits intomarshmallow-code:devfrom
greyli:add-deprecated

Conversation

@greyli
Copy link
Copy Markdown
Contributor

@greyli greyli commented Jun 25, 2021

I didn't add the test since it's a manual field like description, example, and externalDocs.

By the way, it's a new field in OAS 3, should we do some special process for it? Maybe something like this:

def field2deprecated(self, field, **kwargs):
    attributes = {}
    if "deprecated" in field.metadata and self.openapi_version.major >= 3:
        attributes["deprecated"] = True
    return attributes

If needed, I will add this method and the tests for it.

@lafrech
Copy link
Copy Markdown
Member

lafrech commented Jun 28, 2021

Does deprecated apply to a property?

Would it make sense to write

    class Schema(ma.schema):
        field = ma.fields.Field(metadata={"deprecated": True})

?

Regarding the v2 vs. v3, we tend to not perform checks and rely on the user to do what is right. In this case, maybe it's wrong and what you propose is the way to go. If we end up doing this, perhaps we could add a x-deprecated in the v2 case while we're at it.

@greyli
Copy link
Copy Markdown
Contributor Author

greyli commented Jun 28, 2021

Does deprecated apply to a property?

Yes. That's exactly what this PR support to achieve:

field = ma.fields.Field(metadata={"deprecated": True})

Since the deprecated is not listed in the _VALID_PROPERTIES list, the user can't pass it through the metadata dict.

@lafrech
Copy link
Copy Markdown
Member

lafrech commented Jun 28, 2021

I understand. I just didn't find an explicit reference to this in the OpenAPI spec. It says

Specifies that a schema is deprecated and SHOULD be transitioned out of usage.

I didn't find an example where it is used on a single property, not the whole schema. This SO answer provides such an example.

@lafrech
Copy link
Copy Markdown
Member

lafrech commented Jun 28, 2021

What do you think about this?

def field2deprecated(self, field, **kwargs):
    attributes = {}
    if "deprecated" in field.metadata:
        if self.openapi_version.major < 3:
            attributes["x-deprecated"] = True
        else:
            attributes["deprecated"] = True
    return attributes

@greyli
Copy link
Copy Markdown
Contributor Author

greyli commented Jun 28, 2021

If we are going to add an x- version for v2, the field2write_only method for writeOnly should have the same processing. Actually, I'm OK to let the user control this property since it has nothing to do with other field arguments (not like load_only -> writeOnly or allow_none -> nullable).

@lafrech
Copy link
Copy Markdown
Member

lafrech commented Jun 28, 2021

Yeah. The only case I can think of is someone with a code base that can be used with both v2 and v3 versions dynamically.

But let's not overthink this. It can always be done later if someone complains.

@lafrech lafrech merged commit 5cf763e into marshmallow-code:dev Jun 28, 2021
@greyli greyli deleted the add-deprecated branch June 28, 2021 08:34
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