#35060 closed Cleanup/optimization (fixed)
Make Model.save() arguments keyword-only
Reported by: | Jacob Walls | Owned by: | Salvo Polizzi |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Adam Johnson, David Wobrock | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Following this forum discussion, I suggest making Model.save()
accept keyword arguments only (following a deprecation period, of course).
For users who need to patch update_fields
in an overridden save()
, it would be much simpler to only have to drill into **kwargs
and not also drill into (and check the length of) *args
first.
Implementing this ticket would alleviate a small part of a current inconsistency in the advice for overridding save(). In one portion, the implementer is advised to pass through both *args
and **kwargs
:
It's also important that you pass through the arguments that can be passed to the model method -- that's what the ``*args, **kwargs`` bit does. Django will, from time to time, extend the capabilities of built-in model methods, adding new arguments. If you use ``*args, **kwargs`` in your method definitions, you are guaranteed that your code will automatically support those arguments when they are added.
Then, an example tailored to passing through update_fields
, passes through *no* variadic arguments:
def save( self, force_insert=False, force_update=False, using=None, update_fields=None ):
We could make the situation better by taking away swallowed *args
as a vector for bugs.
Change History (21)
comment:1 by , 12 months ago
Component: | Uncategorized → Database layer (models, ORM) |
---|
comment:2 by , 12 months ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:4 by , 12 months ago
I haven't started, so if you'd like to assign the ticket to yourself, that's fine with me. (Be sure to follow the "Deprecating a feature" instructions and update the two documentation examples I mentioned above.)
follow-up: 6 comment:5 by , 12 months ago
Cc: | added |
---|
Nice one. If this ticket goes well, it might serve as a template for making some other functions keyword-only, such as models.Field.__init__
(except the first two args, verbose_name
and name
).
comment:6 by , 12 months ago
Replying to Adam Johnson:
Nice one. If this ticket goes well, it might serve as a template for making some other functions keyword-only, such as
models.Field.__init__
(except the first two args,verbose_name
andname
).
We already have "templates" for such deprecations e.g. ad18a0102cc2968914232814c6554763f15abbe3 or b8738aea14446b267a47087b52b38a98b440a6aa.
comment:7 by , 12 months ago
Has patch: | set |
---|
follow-up: 9 comment:8 by , 12 months ago
Has patch: | unset |
---|
I'm sorry but some tests do not work, so I temporarily closed PR. I will fix them
comment:9 by , 12 months ago
Has patch: | set |
---|
Replying to Salvo Polizzi:
I'm sorry but some tests do not work, so I temporarily closed PR. I will fix them
comment:10 by , 12 months ago
Cc: | added |
---|---|
Patch needs improvement: | set |
comment:11 by , 12 months ago
Patch needs improvement: | unset |
---|
comment:12 by , 12 months ago
Owner: | changed from | to
---|---|
Patch needs improvement: | set |
comment:13 by , 12 months ago
Patch needs improvement: | unset |
---|
comment:14 by , 12 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
Sounds reasonable.