Opened 8 weeks ago
Last modified 7 weeks ago
#35758 assigned Cleanup/optimization
Use keyword argument rather than a positional argument for on_delete in the ForeignKey.on_delete docs.
Reported by: | kay | Owned by: | kay |
---|---|---|---|
Component: | Documentation | Version: | 5.1 |
Severity: | Normal | Keywords: | ForeignKey, code examples |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
The code snippet for `ForeignKey.on_delete` in the docs does not spell out on_delete
, which looks odd and can be confusing.
While further up, at the start of the ForeignKey
section, it is explained to be a positional argument, the subsection documenting the argument itself doesn't seem like the best place to demonstrate that it isn't actually a keyword argument. I'd therefore suggest to add it in.
For variety/to remind users of its positional nature, it could instead be removed from any of the preceding code blocks, which all include it. The `products/models.py` example might make sense as it's only the second example in the overall ForeignKey
section but identical to the first example.
Relatedly, the intro to the Arguments section, which leads into on_delete
, is ambiguously worded. It says, "ForeignKey
accepts other arguments that define the details of how the relation works." (emphasis mine), suggesting what's next are optional arguments or ones not yet discussed... when on_delete
is neither.
Change History (6)
follow-up: 5 comment:1 by , 7 weeks ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 7 weeks ago
Summary: | ForeignKey.on_delete example confusing (missing argument it's meant to document) → Use keyword argument rather than a positional argument for on_delete in the ForeignKey.on_delete docs. |
---|
comment:3 by , 7 weeks ago
on_delete
was an optional argument until Django 2.0 (#21127, ddd3268975dca9094d94ab1df56dae0a24a58865). At that time, the tests were updated to remove on_delete=
(for brevity) and on_delete=
was added to the documentation examples as a keyword argument (for backward compatibility with older Django versions).
I think some of the confusion comes from documentation that wasn't completely updated at that time.
Since on_delete
has now been a required argument for years, I think there's no more value in advocating it be a keyword argument in model definitions.
comment:4 by , 7 weeks ago
In the same vein as we required all argument of form fields to be keyword arguments, I'd also be in favor of having mostly keyword arguments for model fields. In my personal practice, only the verbose_name is used as first positional argument.
comment:5 by , 7 weeks ago
Replying to Tim Graham:
Since
on_delete
has now been a required argument for years, I think there's no more value in advocating it be a keyword argument in model definitions.
Replying to Claude Paroz:
In the same vein as we required all argument of form fields to be keyword arguments, I'd also be in favor of having mostly keyword arguments for model fields. In my personal practice, only the verbose_name is used as first positional argument.
Just to clarify: I wasn't looking to start a discussion about argument types on model fields, or to advocate one type over another. This is solely about the docs and code snippets/examples contained within them. Further discussions about implementation should probably go in a separate ticket.
Replying to Sarah Boyce:
Agreed. Would you like to provide a patch?
I would say this doesn't need a ticket but as you've raised one, accepting
Yeah, I gathered that the suggested change may be small and "insignificant" enough to not necessitate a ticket, but I didn't want to stuff a bunch of explanations in a PR and potentially have to back and forth there & generally thought it wise(r) to follow the proper workflow considering I haven't contributed before. (The CONTRIBUTING file on GitHub warns that "anything more than fixing a typo" is considered a non-trivial PR.)
Will get working on a patch!
comment:6 by , 7 weeks ago
Owner: | set to |
---|---|
Status: | new → assigned |
Agreed. Would you like to provide a patch?
I would say this doesn't need a ticket but as you've raised one, accepting