[Django] #21042: Inconsistent __get__() implementation: SingleRelatedObjectDescriptor VS FileDescriptor
Django
FileDescriptor
----------------------------------------------+--------------------
Reporter: benoitbryon | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: 1.5
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
I was reading code to understand why Sphinx's autodoc fails to handle some
model attributes such as FileField. See
https://bitbucket.org/birkenfeld/sphinx/issue/1254/autodoc-fails-to-
handle-descriptors-with for details...
... then I wondered why SingleRelatedObjectDescriptor.__get__() returns
self if instance is None whereas FileDescriptor.__get__() raises
AttributeError().
I guess they should have the same behaviour. But I'm not sure since I do
not know the reason why they differ...
As I understand
[http://docs.python.org/2/reference/datamodel.html#object.__get__],
raising AttributeError is a feature.
But as mentioned in https://github.com/deschler/django-
modeltranslation/pull/131, returning self could be a solution to get
autodoc work (but isn't it a hack?).
Notes and references:
* SingleRelatedObjectDescriptor's implementation:
https://github.com/django/django/blob/c7d0ff0cad24dbf444f2e4b534377c3352c7aa98/django/db/models/fields/related.py#L182
* FileDescriptor's implementation:
https://github.com/django/django/blob/31e6d58d46894ca35080b4eab7967e4c6aae82d4/django/db/models/fields/files.py#L157
* Creator (and thus many custom fields, including third-party such as
django-uuidfield) is also affected:
https://github.com/django/django/blob/03d9566e0df45c72bffa99fe244a92f0279da56f/django/db/models/fields/subclassing.py#L33
--
Ticket URL: <https://code.djangoproject.com/ticket/21042>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
FileDescriptor
Reporter: benoitbryon | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by benoitbryon):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Does #11353 refers to the same problem?
> After this modification I can use autodoc however I am not sure if this
is not breaking something else or only hiding the problem.
--
Ticket URL: <https://code.djangoproject.com/ticket/21042#comment:1>
Django
FileDescriptor
Reporter: benoitbryon | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by bmispelon):
* cc: bmispelon@… (added)
* stage: Unreviewed => Accepted
Comment:
Hi,
I did some digging, and it appears that the `return self` line in
`SingleRelatedObjectDescriptor.__get__` was added in commit
5ef0c03ae9aca99289737ba6d88a371ad95cf432 as a fix for #8248.
As for `FileDescriptor.__get__`, as far as I can tell it was introduced in
7899568e01fc9c68afe995fa71de915dd9fcdd76 and the initial implementation
already raised an `AttributeError`.
So all in all, it seems that the inconsitency was not intended and fixing
it with a `return self` might be the correct way to go (as the commit
message for 5ef0c03ae9aca99289737ba6d88a371ad95cf432 mentions, this is
supposedly the way that the builtin `property` does it).
I think it would also be worth it to check if there are other descriptor
in django's code that have the same issue and we could also add a comment
to clarify the intent of that particular line.
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/21042#comment:2>
Django
FileDescriptor
Reporter: benoitbryon | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by bmispelon):
For reference, I found a reference for the `return self` in python's
source code:
http://hg.python.org/cpython/file/1d88d04aade2/Objects/descrobject.c#l1243
--
Ticket URL: <https://code.djangoproject.com/ticket/21042#comment:3>
Django
FileDescriptor
Reporter: benoitbryon | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: jm.barbier+djangocode@… (added)
Comment:
Hello
changing the ̀`AttributeError` to `return self` effectively solves
Sphinx's autodoc problem.
Running the entire django 1.5.5 test suite with this modification does not
raise any problem except of course in
`modeltests.files.tests.FileStorageTests` (the
`self.assertRaises(AttributeError, lambda: Storage.normal)` assertion on
line 25)..
It would be great to have this change in 1.6
Thanks
--
Ticket URL: <https://code.djangoproject.com/ticket/21042#comment:4>
Django
implementation
-------------------------------------+-------------------------------------
Reporter: benoitbryon | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/5377 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/21042#comment:5>
Django
implementation
-------------------------------------+-------------------------------------
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/21042#comment:6>
Django
implementation
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: Database layer | Version: 1.5
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"9f6b704769ba5bc0daafc25340d3dc28b18d8fb1" 9f6b7047]:
{{{
#!CommitTicketReference repository=""
revision="9f6b704769ba5bc0daafc25340d3dc28b18d8fb1"
Fixed #21042 -- Allowed accessing FileDescriptor on the model class.
This is consistent with ability to reference other descriptors
on the model class (5ef0c03ae9aca99289737ba6d88a371ad95cf432).
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21042#comment:7>