#21940 closed Cleanup/optimization (fixed)
Consistent contribute_to_class's virtual_only
Reported by: | German M. Bravo | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 1.6 |
Severity: | Normal | Keywords: | |
Cc: | mmitar@…, jernej@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
I needed to create a field like class for my django project and I wanted it to be a virtual field... virtual_only
is there for that reason, but my field was inheriting from FileField
, and FileField.contribute_to_class()
doesn't take virtual_only
and so it doesn't passes it to Field.contribute_to_class()
. I believe passing virtual_only
attribute should be consistent across the source code. I'm attaching a simple patch to fix this. One warning would be for custom fields made by users which have their own contribute_to_class
(those will need to be changed to properly receive the virtual_only
parameter as optional.)
Attachments (1)
Change History (11)
by , 11 years ago
Attachment: | #21940-virtual_only_in_contribute_to_class.diff added |
---|
comment:1 by , 11 years ago
comment:2 by , 11 years ago
Component: | Uncategorized → Database layer (models, ORM) |
---|---|
Has patch: | set |
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Cleanup/optimization |
comment:3 by , 11 years ago
IMO what is done is better, because
In the contribute_to_class in Field class:
def contribute_to_class(self, cls, name, virtual_only=False)
If from subclass:
super(AutoField, self).contribute_to_class(cls, name, **kwargs)
There is only virtual_only as keyword argument, if we pass **kwargs
then it will be possible to pass any other variable to it as well, which is wrong I believe.
comment:4 by , 10 years ago
Cc: | added |
---|
comment:5 by , 10 years ago
GeoDjango is also missing virtual_only
in their fields: https://github.com/django/django/blob/master/django/contrib/gis/db/models/fields.py#L213
comment:6 by , 10 years ago
Cc: | added |
---|
comment:8 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:10 by , 10 years ago
Definitely not 1.6, it's in security-fixes-only mode. And I don't think this meets the criteria in our backporting policy for backport to 1.7 either.
Would it be better to simply accept and pass on
**kwargs
? I think that would eliminate the minor backwards compatibility concerns as well.