#34373 closed Cleanup/optimization (wontfix)

Update docs on ForeignKey to suggest setting "to" and object before a string

Reported by: Salaah Amin Owned by:
Component: Documentation Version: 4.1
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

In the documentation around model ForeignKeys, the documentation explains how to set to to a string which references a model before showing how an actual model can be referenced in the on_delete section.

I propose adding a section before the string version, explaining how the ForeignKey can be used using a reference to a model using the model class itself. This is purely to avoid defaulting to the string method which makes it easy to fall into the circular dependencies later down the line.

So in other words, rather than jumping into:

class Car(models.Model):
    manufacturer = models.ForeignKey(
        'Manufacturer',
        on_delete=models.CASCADE,
    )
    # ...

It may be helpful to show the following beforehand:

class Car(models.Model):
    manufacturer = models.ForeignKey(
        Manufacturer,
        on_delete=models.CASCADE,
    )
    # ...

In addition, I propose adding some additional information suggesting (if we do agree on this) that circular dependencies should be avoided, especially as it can mess up migration orders and make it difficult to squash migrations at a later stage.

Change History (6)

comment:1 by Salaah Amin, 22 months ago

Type: UncategorizedCleanup/optimization

in reply to:  description ; comment:2 by Mariusz Felisiak, 22 months ago

Resolution: wontfix
Status: newclosed

Replying to Salaah Amin:

In the documentation around model ForeignKeys, the documentation explains how to set to to a string which references a model before showing how an actual model can be referenced in the on_delete section.

Thanks for this ticket, however I see no reason to change other ForeignKey examples, in most cases there is nothing wrong in using model classes.

I propose adding a section before the string version, explaining how the ForeignKey can be used using a reference to a model using the model class itself. This is purely to avoid defaulting to the string method which makes it easy to fall into the circular dependencies later down the line.

This is already noted in this section: "This sort of reference, called a lazy relationship, can be useful when resolving circular import dependencies between two applications.", IMO no additional section is needed.

in reply to:  2 comment:3 by Salaah Amin, 22 months ago

Resolution: wontfix
Status: closednew

I would argue that there is some value in adding it as proposed.

IMO notes are easy to miss and so it's easy to miss the fact that the string method can be considered when circular deps. become an issue.
I feel that it's better to reference an actual model and the have the warning appear about circular deps. The user then has the prompt to either go down the string method, or avoid circular dependencies altogether between the apps. This hopefully would help others write more maintainable Django code as dealing with circular deps. when trying to squash migrations (what let me write this in the first place) is a real pain.

Replying to Mariusz Felisiak:

Replying to Salaah Amin:

In the documentation around model ForeignKeys, the documentation explains how to set to to a string which references a model before showing how an actual model can be referenced in the on_delete section.

Thanks for this ticket, however I see no reason to change other ForeignKey examples, in most cases there is nothing wrong in using model classes.

I propose adding a section before the string version, explaining how the ForeignKey can be used using a reference to a model using the model class itself. This is purely to avoid defaulting to the string method which makes it easy to fall into the circular dependencies later down the line.

This is already noted in this section: "This sort of reference, called a lazy relationship, can be useful when resolving circular import dependencies between two applications.", IMO no additional section is needed.

comment:4 by Samith Karunathilake, 22 months ago

Owner: changed from nobody to Samith Karunathilake
Status: newassigned

comment:5 by Samith Karunathilake, 22 months ago

Owner: Samith Karunathilake removed
Status: assignednew

comment:6 by Mariusz Felisiak, 22 months ago

Resolution: wontfix
Status: newclosed

I appreciate you'd like to reopen the ticket, but please follow the triaging guidelines with regards to wontfix tickets and take this to DevelopersMailingList.

Note: See TracTickets for help on using tickets.
Back to Top