Opened 9 years ago

Closed 9 years ago

#25478 closed Bug (worksforme)

1.8 migrating doc code sample needs parens

Reported by: Ned Batchelder Owned by: nobody
Component: Documentation Version: 1.8
Severity: Normal Keywords:
Cc: pirosb3@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I think this sample from https://docs.djangoproject.com/en/1.8/ref/models/meta/#migrating-from-the-old-api needs parens to be correct. The docs have:

[
    (f, f.model if f.model != MyModel else None)
    for f in MyModel._meta.get_fields()
    if not f.is_relation
        or f.one_to_one
        or (f.many_to_one and f.related_model)
]

but the condition seems wrong. Shouldn't it be:

[
    (f, f.model if f.model != MyModel else None)
    for f in MyModel._meta.get_fields()
    if not (f.is_relation
        or f.one_to_one
        or (f.many_to_one and f.related_model)
    )
]

Change History (6)

comment:1 by Claude Paroz, 9 years ago

Resolution: worksforme
Status: newclosed

Just tried an expression like this in a shell and apparently the parens are not mandatory in this case.

>>> lst = [1, 2, 3, 'a']
>>> print([
...     item
...     for item in lst
...     if item > 0
...         and item != 'a'
... ])
[1, 2, 3]

comment:2 by Ned Batchelder, 9 years ago

Resolution: worksforme
Status: closednew

I didn't mean that it was a syntax error. I meant that the logic was wrong. Without the parens, the condition is equivalent to:

(not f.is_relation) or (f.one_to_one) or (f.many_to_one and f.related_model)

Don't we actually want:

not (f.is_relation or f.one_to_one or (f.many_to_one and f.related_model))

That is, the field should be excluded if it's a relation or if it's one-to-one, or if it's both many-to-one and related.

comment:3 by Claude Paroz, 9 years ago

Cc: pirosb3@… added
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

Oh, sorry for misunderstanding! I think you are right.

comment:4 by Tim Graham, 9 years ago

It looks correct to me. The idea seems to be to exclude all relations except foreign keys and one-to-one fields. Here's an example from djangoproject.com where you can see those fields included in the old API:

>>> from accounts.models import Profile
>>> Profile._meta.get_fields_with_model()
[(<django.db.models.fields.AutoField: id>, None), (<django.db.models.fields.related.OneToOneField: user>, None), (<django.db.models.fields.CharField: name>, None)]

Can you provide an example discrepancy?

comment:5 by Daniel Pyrathon, 9 years ago

It looks correct to me too. To make sure we could also write an automated test for it if there isn't one already.

comment:6 by Tim Graham, 9 years ago

Resolution: worksforme
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top