Opened 9 months ago

Closed 9 months ago

Last modified 11 days ago

#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 Jacob Walls, 9 months ago

Component: UncategorizedDatabase layer (models, ORM)

comment:2 by Mariusz Felisiak, 9 months ago

Triage Stage: UnreviewedAccepted

Sounds reasonable.

comment:3 by Aryan Gholizadeh Mojaveri, 9 months ago

May we create a PR for this now?

comment:4 by Jacob Walls, 9 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.)

comment:5 by Adam Johnson, 9 months ago

Cc: Adam Johnson 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).

in reply to:  5 comment:6 by Mariusz Felisiak, 9 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 and name).

We already have "templates" for such deprecations e.g. ad18a0102cc2968914232814c6554763f15abbe3 or b8738aea14446b267a47087b52b38a98b440a6aa.

comment:7 by Salvo Polizzi, 9 months ago

Has patch: set
Last edited 9 months ago by Salvo Polizzi (previous) (diff)

comment:8 by Salvo Polizzi, 9 months ago

Has patch: unset

I'm sorry but some tests do not work, so I temporarily closed PR. I will fix them

in reply to:  8 comment:9 by Salvo Polizzi, 9 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 David Wobrock, 9 months ago

Cc: David Wobrock added
Patch needs improvement: set

comment:11 by Salvo Polizzi, 9 months ago

Patch needs improvement: unset

comment:12 by Jacob Walls, 9 months ago

Owner: changed from Jacob Walls to Salvo Polizzi
Patch needs improvement: set

comment:13 by Salvo Polizzi, 9 months ago

Patch needs improvement: unset

comment:14 by Mariusz Felisiak, 9 months ago

Triage Stage: AcceptedReady for checkin

comment:15 by Mariusz Felisiak <felisiak.mariusz@…>, 9 months ago

Resolution: fixed
Status: assignedclosed

In 3915d4c:

Fixed #35060 -- Deprecated passing positional arguments to Model.save()/asave().

comment:16 by GitHub <noreply@…>, 3 months ago

In 28522c3:

Fixed #35554, Refs #35060 -- Corrected deprecated *args parsing in Model.save()/asave().

The transitional logic added to deprecate the usage of *args for
Model.save()/asave() introduced two issues that this branch fixes:

  • Passing extra positional arguments no longer raised TypeError.
  • Passing a positional but empty update_fields would save all fields.

Co-authored-by: Natalia <124304+nessita@…>

comment:17 by Natalia <124304+nessita@…>, 3 months ago

In 38717291:

[5.1.x] Fixed #35554, Refs #35060 -- Corrected deprecated *args parsing in Model.save()/asave().

The transitional logic added to deprecate the usage of *args for
Model.save()/asave() introduced two issues that this branch fixes:

  • Passing extra positional arguments no longer raised TypeError.
  • Passing a positional but empty update_fields would save all fields.

Co-authored-by: Natalia <124304+nessita@…>

Backport of 28522c3c8d5eb581347aececc3ac61c134528114 from main.

comment:18 by nessita <124304+nessita@…>, 4 weeks ago

In 52ed2b64:

Refs #35060 -- Adjusted deprecation warning stacklevel in Model.save()/asave().

comment:19 by Natalia <124304+nessita@…>, 4 weeks ago

In c87007ab:

[5.1.x] Refs #35060 -- Adjusted deprecation warning stacklevel in Model.save()/asave().

Backport of 52ed2b645e1dd8c9a874cfd21c4c9f2500032626 from main.

comment:20 by Sarah Boyce <42296566+sarahboyce@…>, 11 days ago

In 38c20651:

Refs #35060 -- Fixed the update to update_fields in overridden save() method docs.

Regression in 3915d4c70d0d7673abe675525b58117a5099afd3.

comment:21 by Sarah Boyce <42296566+sarahboyce@…>, 11 days ago

In bf45f067:

[5.1.x] Refs #35060 -- Fixed the update to update_fields in overridden save() method docs.

Regression in 3915d4c70d0d7673abe675525b58117a5099afd3.

Backport of 38c206515494cb28c48f77c10145a8aa9a172629 from main.

Note: See TracTickets for help on using tickets.
Back to Top