Opened 5 years ago

Closed 2 years ago

#30947 closed Cleanup/optimization (fixed)

Apply data structure best practices to the django.contrib models and docs

Reported by: Jon Dufresne Owned by: Alex Morega
Component: Documentation Version: dev
Severity: Normal Keywords:
Cc: Johannes Maron Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

In PR https://github.com/django/django/pull/11267 an argument is presented that explains why list is often the best choice for Model.Meta data structures. This landed as https://github.com/django/django/commit/97d3321e89c8d4434927bdbc308db1ccffa99d3b.

Now change all models in django.contrib to follow our own stated best practices. People often look at existing examples for how to write new code, so we might as well promote the standards we document.

Change History (22)

comment:1 by Johannes Maron, 5 years ago

Cc: Johannes Maron added
Component: UncategorizedDocumentation
Has patch: set
Owner: changed from nobody to Jon Dufresne
Status: newassigned
Triage Stage: UnreviewedAccepted

comment:2 by Mariusz Felisiak, 5 years ago

Patch needs improvement: set

comment:3 by Jon Dufresne, 5 years ago

Patch needs improvement: unset

Updated PR.

comment:4 by Carlos_Mir_de_Souza, 5 years ago

Triage Stage: AcceptedReady for checkin

It`s ok.

comment:5 by Jon Dufresne <jon.dufresne@…>, 5 years ago

In e5cacb1:

Refs #30947 -- Changed tuples to lists in model Meta options examples in docs.

Follow up to 97d3321e89c8d4434927bdbc308db1ccffa99d3b.

comment:6 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In b9fe7f92:

Fixed #30947 -- Changed tuples to lists in model Meta options in django.contrib modules.

The Django "Model Meta options" docs provide examples and generally
point the reader to use lists for the unique_together and ordering
options. Follow our own advice for contrib models.

More generally, lists should be used for homogeneous sequences of
arbitrary lengths of which both unique_together and ordering are.

comment:7 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In 2000ed51:

[3.0.x] Refs #30947 -- Changed tuples to lists in model Meta options examples in docs.

Follow up to 97d3321e89c8d4434927bdbc308db1ccffa99d3b.

Backport of e5cacb1f47cb3a2943bbc7685a630c84503b0d1b from master

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In a7f90d9:

[2.2.x] Refs #30947 -- Changed tuples to lists in model Meta options examples in docs.

Follow up to 97d3321e89c8d4434927bdbc308db1ccffa99d3b.

Backport of e5cacb1f47cb3a2943bbc7685a630c84503b0d1b from master

comment:9 by Mariusz Felisiak, 4 years ago

Summary: Apply data structure best practices to the django.contrib modelsApply data structure best practices to the django.contrib models and docs

#32280 was a duplicate for extra docs changes.

Last edited 2 years ago by Mariusz Felisiak (previous) (diff)

comment:10 by Mariusz Felisiak, 4 years ago

Has patch: unset
Resolution: fixed
Status: closednew
Triage Stage: Ready for checkinAccepted

comment:11 by Mariusz Felisiak, 2 years ago

Owner: Jon Dufresne removed
Status: newassigned

comment:12 by Mariusz Felisiak, 2 years ago

Status: assignednew

comment:13 by Mariusz Felisiak, 2 years ago

Easy pickings: set

comment:16 by Ojas Gupta, 2 years ago

Owner: set to Ojas Gupta
Status: newassigned

Ok so I have to convert tuple of list to list of list.

comment:17 by Alex Morega, 2 years ago

This is actually a pet peeve of mine; would love to see the docs updated to using lists instead of tuples.

Would this be a good checklist of places to update in the codebase and docs? https://gist.github.com/mgax/32318118a71ce602a00a80daeecb60fc

comment:18 by Simon Charette, 2 years ago

Some of the examples you pointed should remain tuples as they are mixing heterogenous data and mostly used to a define a schema over data.

Since we're on the topic of data structure best practices unique_together and friends should likely be defined as set and not list as only unique entries will be considered and because the order in which they are defined on the model doesn't have any significance.

comment:19 by Alex Morega, 2 years ago

Some of the examples you pointed should remain tuples as they are mixing heterogenous data and mostly used to a define a schema over data.

Right! I meant it as a starting point of places that might need fixing.

Since we're on the topic of data structure best practices unique_together and friends should likely be defined as set and not list as only unique entries will be considered and because the order in which they are defined on the model doesn't have any significance.

I see your point, but AFAIU, a side effect of unique_together is creating composite database indexes, where the order of columns matters.

There hasn't been any activity on the ticket for 3 weeks; may I take ownership?

comment:20 by Simon Charette, 2 years ago

I see your point, but AFAIU, a side effect of unique_together is creating composite database indexes, where the ​order of columns matters.

The order of columns within an index or unique constraints matter, but the order of constraints doesn't. This obviously means that you can't use the flattened syntax index_together = {'foo', 'bar'} but favor index_together = {('foo', 'bar')}.

comment:21 by Alex Morega, 2 years ago

Has patch: set
Owner: changed from Ojas Gupta to Alex Morega

comment:22 by Mariusz Felisiak, 2 years ago

Triage Stage: AcceptedReady for checkin

comment:23 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In de6c9c7:

Refs #30947 -- Changed tuples to lists where appropriate.

comment:24 by Mariusz Felisiak, 2 years ago

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top