Skip to content

Add missing decorators: @versioning_class(), @content_negotiation_class(), @metadata_class() #9719

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

qqii
Copy link

@qqii qqii commented Jun 16, 2025

This adds the three missing decorators, fixing #9716.

  • @content_negotiation_class()
  • @metadata_class()
  • @versioning_class()

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@browniebroke
Copy link
Member

Happy to add the others with some support, as I've not used content_negotiation_class or metadata_class.

Yes, I think that if you add one, it makes sense to add all 3 that you found were missing 👍🏻

@browniebroke browniebroke added this to the 3.17 milestone Aug 1, 2025
@qqii qqii force-pushed the version-decorator branch from 671d36b to 96223eb Compare August 5, 2025 18:11
@qqii qqii changed the title Add missing decorator: @versioning_class() Add missing decorators: @versioning_class(), @content_negotiation_class(), @metadata_class() Aug 5, 2025
@qqii
Copy link
Author

qqii commented Aug 5, 2025

Sure @browniebroke, I've amended this PR to add all of them. Do the tests for content_negotiation_class and metadata_class suffice?

browniebroke
browniebroke previously approved these changes Aug 9, 2025
Copy link
Member

@browniebroke browniebroke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@auvipy auvipy self-requested a review August 10, 2025 04:14
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may be we have to update the relevant docs for these, as well?

@@ -186,6 +186,9 @@ The available decorators are:
* `@authentication_classes(...)`
* `@throttle_classes(...)`
* `@permission_classes(...)`
* `@content_negotiation_class(...)`
* `@metadata_class(...)`
* `@versioning_class(...)`

Each of these decorators takes a single argument which must be a list or tuple of classes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That statement needs to be updated, the ones we're adding are accepting a single class, not a list or tuple of classes.

Copy link
Author

@qqii qqii Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. My guess is the original wording is meant to clarify that they should be used like

@api_view(["GET"])
@renderer_classes([RenderA, RenderB])
def view(request):
    pass

rather than

@api_view(["GET"])
@render_classes(RenderA, RenderB)
def view(request):
    pass

How about something like:

Each of these decorators is equivalent to setting their respective api policy attributes.
Decorators that end with classes take a single argument which must be a list or tuple of classes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start, I would go one step further and explicitly outline what each form expect. Something along the lines of:

Each of these decorators is equivalent to setting their respective api policy attributes.
All decorators take a single argument. The ones that end with _class expect a single class while the ones ending in _classes expect a list or tuple of classes.

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.

3 participants