Opened 3 years ago

Closed 3 years ago

#22217 closed Bug (fixed)

ManyToManyField.through_fields docs mix up description of arguments

Reported by: Akis Kesoglou Owned by: Ramiro Morales
Component: Documentation Version: master
Severity: Release blocker Keywords:
Cc: akiskesoglou@…, loic@… Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The patch for ticket #14549 got the given example and description of ('field1', 'field2') mixed up.

The error lies in this paragraph: https://github.com/django/django/commit/c627da0ccc12861163f28177aa7538b420a9d310#diff-7e6909c7694e8a3ae170c2a22f9b6e0cR1370

Change History (8)

comment:1 Changed 3 years ago by Akis Kesoglou

Cc: akiskesoglou@… added
Has patch: set
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Severity: NormalRelease blocker
Summary: ManyToMany.through_fields docs mix up description of argumentsManyToManyField.through_fields docs mix up description of arguments

I’m taking the liberty to mark as a release blocker as the docs are currently misleading users.

Made a pull request: https://github.com/django/django/pull/2402

comment:2 Changed 3 years ago by Akis Kesoglou

Owner: changed from nobody to Akis Kesoglou
Status: newassigned

comment:3 Changed 3 years ago by loic84

I noticed that E332 was shadowed when through_fields was set, apparently it's fixed with this patch.

Regarding the new ValueError, any reason it can't be made a system check as well?

comment:4 Changed 3 years ago by loic84

Cc: loic@… added

comment:5 Changed 3 years ago by Akis Kesoglou

System checks I believe are performed too early -- the relation has not been setup at this point -- though it would be great if I'm mistaken, as I also prefer for it to be a check.

comment:6 Changed 3 years ago by Akis Kesoglou

Updated the pull request. This patch improves on many areas:

  • Fixes the docs for through_fields.
  • Moves validation earlier, in the check() method, as suggested by loic84.
  • Rewords some validation error messages and provides hints for some of them.

All tests pass.

I've squashed commits to a larger one, let me know if i should create separate tickets and/or commits for each issue fixed.

Version 0, edited 3 years ago by Akis Kesoglou (next)

comment:7 Changed 3 years ago by Ramiro Morales

Owner: changed from Akis Kesoglou to Ramiro Morales

comment:8 Changed 3 years ago by Ramiro Morales <cramm0@…>

Resolution: fixed
Status: assignedclosed

In aaad3e27ac7cfcbbfeac6353d17d27e8da523cc8:

Fixed #22217 - ManyToManyField.through_fields fixes.

  • Docs description of arguments mix up.
  • Keep it from erroneously masking E332 check.
  • Add checks E338 and E339, tweak message of E337.
Note: See TracTickets for help on using tickets.
Back to Top