Make Field constructors keyword-only#7632
Conversation
akx
force-pushed
the
field-kwargs-only
branch
from
ffe82b8 to
25ead1d
Compare
auvipy
left a comment
auvipy
left a comment
There was a problem hiding this comment.
could you please fix the conflicts?
|
@auvipy Rebased. |
|
@auvipy Can we get this merged? :) |
|
I am not maintainer :D |
|
Fair. @tomchristie or someone else, can you take a look? |
|
Yup seems pretty reasonable. |
|
Based on past experience I'd expect that someone somewhere may well have some code updating to do after this change, but let's see. And yes it clearly makes sense. |
And there we go: fdb4931#r54532785 We might reconsider this given that we've already seen a user hit a pain point with the change, even without having released a new version. Unsure. |
|
documenting this would be the right choice, IMHO |
|
Cc @afparsons for having bumped into that issue in fdb4931#r54532785: I believe the change in this PR is net positive and will likely uncover some bugs in downstream code even if it might need some work to adapt. In that sense I agree with @auvipy that it'd be better to just document this loudly for the next release. |
This reverts commit fdb4931.
|
I'm extremely late to the party but this now makes unclear if this was intended -- but it certainly strikes me as odd |
see encode/django-rest-framework#7632 <!-- Describe your PR here. -->
see encode/django-rest-framework#7632 <!-- Describe your PR here. -->
4 participants
Description
This PR makes the Field constructor keyword-only by adding a
*(and turning*args, **kwargsinto just**kwargsin subclasses). I don't believe anyone is (or should be) constructing fields asserializers.CharField(False, True, False, serializers.empty, ...)The root issue this fixes is the nonobvious problem in e.g.
– you wouldn't expect that yields a non-validated read-only list field, would you? 😄 What happens is the positional
serializers.CharField()argument gets passed up into theserializers.Field()call as the first argument, which just so happens to beread_only, and the field instance is a truthy value.../cc @magdapoppins for having initially bumped into this issue.