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

Fix BaseMetadata.determine_metadata type hint #394

Merged
merged 1 commit into from
Apr 28, 2023

Conversation

realsuayip
Copy link
Contributor

The return value of BaseMetadata.determine_metadata() is passed to Response.data, which accepts Any (In my case, I was trying to return None, for empty response body).

@intgr
Copy link
Contributor

intgr commented Apr 27, 2023

The return value of BaseMetadata.determine_metadata() is passed to Response.data, which accepts Any

I don't see the issue with that. determine_metadata() returns dict[str, Any], and that is compatible with Any.

The only implementation of this method is SimpleMetadata.determine_metadata(), which indeed is guaranteed to return a dict.

Please describe the issue you're having.

@intgr intgr self-assigned this Apr 27, 2023
@realsuayip
Copy link
Contributor Author

When subclassing BaseMetadata, I would like to return an empty Response (requiring me to return None in the related method). However, since the base class requires a compatible type with dict[str, Any], this is problematic.

@sobolevn sobolevn merged commit 3bba7ab into typeddjango:master Apr 28, 2023
@intgr
Copy link
Contributor

intgr commented Apr 28, 2023

I'm still not convinced this should have been merged.

Is there a downside if you returned {} instead of None?

Other methods of SimpleMetadata are still typed as dict[str, Any], should we create SimpleMetadata.determine_metadata stub that constrains return to dict[str, Any], so SimpleMetadata's methods are consistent? Or type them all as Any?

@realsuayip
Copy link
Contributor Author

realsuayip commented Apr 28, 2023

The difference is the return value of Response() (empty body) and Response({}) (body with empty object). In practice, it does not make much difference since body of OPTIONS request is mostly ignored. However, if someone requires some custom body for any reason, they should be able to return any Response they want. And since Response allows more than dict[str, Any], (for example, I think, serializer.data does not yield a dict), it makes sense.

I don't think we need to change other metadata methods since they are DRF opinionated methods. If the users require some custom metadata, they can ignore these methods altogether for their implementation (such as the example in the documentation). If they want to override these methods, I assume they will be making smaller changes that should not affect the return type. If they do change the type however, it can be easily circumvented by creating another method (instead of overriding) and calling the method there.

@namper
Copy link
Contributor

namper commented Jun 5, 2023

In theory 'Renderable' object should be passed as data

Wouldn't more suggestive type hint be Dict[str, Any] | None ?

@realsuayip
Copy link
Contributor Author

@namper

In theory, anything that could be json.dumped should be passable. These include things like strings, floats, integers lists etc. (not just dict or None).

As far as I know, typing such type is not so straightforward since it requires arbitrary nesting. There seems to be a partial solution at the end of python/typing#182 but I doubt it would be adopted given the complexity and possible issues it will bring.

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.

4 participants