#22217 closed Bug (fixed)

ManyToManyField.through_fields docs mix up description of arguments

Reported by: dfunckt Owned by: ramiro
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 14 months ago by dfunckt

  • Cc akiskesoglou@… added
  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Severity changed from Normal to Release blocker
  • Summary changed from ManyToMany.through_fields docs mix up description of arguments to ManyToManyField.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 14 months ago by dfunckt

  • Owner changed from nobody to dfunckt
  • Status changed from new to assigned

comment:3 Changed 14 months 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 14 months ago by loic84

  • Cc loic@… added

comment:5 Changed 14 months ago by dfunckt

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 14 months ago by dfunckt

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

  • Fixes the docs for through_fields.
  • Fixes validation of through_fields that would mask other validation errors.
  • 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.

Last edited 14 months ago by dfunckt (previous) (diff)

comment:7 Changed 14 months ago by ramiro

  • Owner changed from dfunckt to ramiro

comment:8 Changed 14 months ago by Ramiro Morales <cramm0@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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