Fix SlugRelatedField.slug_field type: cannot be None
#830
Conversation
intgr
left a comment
intgr
left a comment
There was a problem hiding this comment.
Maybe the parameter should be changed in DRF upstream instead?
|
|
||
| class SlugRelatedField(RelatedField[_MT, str, str]): | ||
| slug_field: str | None | ||
| slug_field: str |
There was a problem hiding this comment.
Changing the attribute is good 👍
rest_framework-stubs/relations.pyi
Outdated
| def __init__( | ||
| self, | ||
| slug_field: str, | ||
| slug_field: str | None = None, |
There was a problem hiding this comment.
But the parameter isn't really | None if there's an assert prohibiting it.
I'd suggest opening a PR to upstream https://github.com/encode/django-rest-framework instead to remove the default value.
There was a problem hiding this comment.
|
I'm not against making the upstream patch, but currently this is the correct typing would you rather wait for upstream to accept my change before accepting a PR to change the typing? |
I think type hints should help write correct code, not so much strictly follow what is written in the function signature. If you cannot pass
The attribute change can be merged immediately. The parameter should be left as-is regardless of whether it's changed upstream. A comment in the |
|
I agree with your sentiment regarding type hints :) I'll fix this PR to include only the attribute change to |
intgr
changed the title
SlugRelatedField.slug_field type: cannot be None
2 participants
I have made things!
as written in the linked issue -
slug_fieldisn't reallyNone, it's onlyNoneas the argument to the__init__, but is asserted to not beNonein the first line of the methodRelated issues
closes #207