Opened 11 years ago

Closed 11 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: dev
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 by Akis Kesoglou, 11 years ago

Cc: akiskesoglou@… added
Has patch: set
Severity: Normal → Release blocker
Summary: ManyToMany.through_fields docs mix up description of arguments → 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 by Akis Kesoglou, 11 years ago

Owner: changed from nobody to Akis Kesoglou
Status: new → assigned

comment:3 by loic84, 11 years ago

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 by loic84, 11 years ago

Cc: loic@… added

comment:5 by Akis Kesoglou, 11 years ago

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 by Akis Kesoglou, 11 years ago

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 11 years ago by Akis Kesoglou (next)

comment:7 by Ramiro Morales, 11 years ago

Owner: changed from Akis Kesoglou to Ramiro Morales

comment:8 by Ramiro Morales <cramm0@…>, 11 years ago

Resolution: → fixed
Status: assigned → 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