#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 (22)
comment:1 by , 23 months ago
| Component: | Uncategorized → Database layer (models, ORM) |
|---|
comment:2 by , 23 months ago
| Triage Stage: | Unreviewed → Accepted |
|---|
comment:4 by , 23 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 , 23 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 , 23 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_nameandname).
We already have "templates" for such deprecations e.g. ad18a0102cc2968914232814c6554763f15abbe3 or b8738aea14446b267a47087b52b38a98b440a6aa.
comment:7 by , 23 months ago
| Has patch: | set |
|---|
follow-up: 9 comment:8 by , 23 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 , 23 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 , 23 months ago
| Cc: | added |
|---|---|
| Patch needs improvement: | set |
comment:11 by , 23 months ago
| Patch needs improvement: | unset |
|---|
comment:12 by , 23 months ago
| Owner: | changed from to |
|---|---|
| Patch needs improvement: | set |
comment:13 by , 23 months ago
| Patch needs improvement: | unset |
|---|
comment:14 by , 23 months ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
Sounds reasonable.