Skip to content
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

Add accepted_renderer, accepted_media_type to Request and Response #649

Merged
merged 2 commits into from
Sep 11, 2024

Conversation

@intgr intgr changed the title Add accepted_renderer, accepted_media_type to request and response Add accepted_renderer, accepted_media_type to Request and Response Sep 11, 2024
Copy link
Contributor

@intgr intgr left a comment

Choose a reason for hiding this comment

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

Thanks! Let's merge this and get a release out the door. We can improve this later.

Comment on lines +51 to +52
accepted_renderer: BaseRenderer | None
accepted_media_type: str | None
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think these can actually be None? Looking at APIView.perform_content_negotiation(), it rather raises exception than returns None.

Same for parse_header_parameters(), it raises NotAcceptable if it can't find a match.

But still this PR is an improvement, I'll merge this and get a release out the door. We can improve this later.

Copy link
Contributor Author

@q0w q0w Sep 12, 2024

Choose a reason for hiding this comment

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

but request may not have these attrs at all. There are hasattr checks after all

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, there's no way to annotate 'possibly missing attribute' for classes. A code comment in pyi is best we can do.

But that's different from None. If the value may be missing but never None then I'd suggest to remove | None. Because that's even more misleading.

@intgr intgr merged commit dc75658 into typeddjango:master Sep 11, 2024
12 checks passed
@q0w q0w deleted the accepted branch September 12, 2024 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants