Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7k
Make Field constructors keyword-only#7632
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 thatsomeonesomewhere 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 Wemight 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 infdb4931#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. |
asottile-sentry commentedJul 21, 2023
I'm extremely late to the party but this now makes unclear if this was intended -- but it certainly strikes me as odd |
seeencode/django-rest-framework#7632<!-- Describe your PR here. -->
seeencode/django-rest-framework#7632<!-- Describe your PR here. -->
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.