-
Notifications
You must be signed in to change notification settings - Fork 91
Development #180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Development #180
Changes from all commits
77bc77f
4c40b0b
4d1990a
b184ade
d17a97d
6c5fab5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,59 +2,83 @@ | |
| Hosts the table tiers, user relations should be derived from. | ||
| """ | ||
|
|
||
| import abc | ||
| from .base_relation import BaseRelation | ||
| from .autopopulate import AutoPopulate | ||
| from .utils import from_camel_case | ||
| from . import DataJointError | ||
|
|
||
|
|
||
| class Part(BaseRelation, metaclass=abc.ABCMeta): | ||
| class classproperty: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we reintroducing
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but I think that last time the case for using them was not as compelling. Let's review that again. Frankly, I did not understand classproperties very well last time and it seemed like then we found an equally good solution without them. In this case, introducing class properties substantially simplified the code in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Last time I brought up the case for stuff like Anyway, I'm just giving you a bit of hard time, and I agree that use of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I had not realized in my ignorance was that to downstream users, class properties can be used the same way as instance properties. At the time, I was under the impression that class properties always had to be called on the class and could not be called on the instance. I also did not realize that when subclassing, each subclass gets its own class property and not just one shared across all subclasses. These features, combined, make it acceptable to use classproperties. Sorry I did not know this. I thought classproperty would introduce all sorts of complications but after checking everything, it seems safe.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, now I realize that my arguments agains classmethods and classproperties were invalid. Thankfully, changing things to classmethods now does not break any user code.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why doesn't Python3 have a standard
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I have no idea why there is no
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. perhaps it is because there might be a confusion with properties of the metaclass. I would like to find a discussion explaining this. |
||
|
|
||
| def __init__(self, f): | ||
| self.f = f | ||
|
|
||
| def __get__(self, obj, owner): | ||
| return self.f(owner) | ||
|
|
||
|
|
||
| class UserRelation(BaseRelation): | ||
| """ | ||
| A subclass of UserRelation defines is a dedicated class interfacing a base relation. | ||
| UserRelation is initialized by the decorator generated by schema(). | ||
| """ | ||
| _connection = None | ||
| _context = None | ||
| _heading = None | ||
|
|
||
| @classproperty | ||
| def connection(cls): | ||
| return cls._connection | ||
|
|
||
| @classproperty | ||
| def full_table_name(cls): | ||
| return r"`{0:s}`.`{1:s}`".format(cls.database, cls.table_name) | ||
|
|
||
|
|
||
| class Part(UserRelation): | ||
| """ | ||
| Inherit from this class if the table's values are details of an entry in another relation | ||
| and if this table is populated by this relation. For example, the entries inheriting from | ||
| dj.Part could be single entries of a matrix, while the parent table refers to the entire matrix. | ||
| Part relations are implemented as classes inside classes. | ||
| """ | ||
| _master = None | ||
|
|
||
| @property | ||
| def master(self): | ||
| if not hasattr(self, '_master'): | ||
| raise DataJointError( | ||
| 'Part relations must be declared inside a base relation class') | ||
| return self._master | ||
| @classproperty | ||
| def master(cls): | ||
| return cls._master | ||
|
|
||
| @property | ||
| def table_name(self): | ||
| return self.master().table_name + '__' + from_camel_case(self.__class__.__name__) | ||
| @classproperty | ||
| def table_name(cls): | ||
| return cls.master.table_name + '__' + from_camel_case(cls.__name__) | ||
|
|
||
|
|
||
| class Manual(BaseRelation, metaclass=abc.ABCMeta): | ||
| class Manual(UserRelation): | ||
| """ | ||
| Inherit from this class if the table's values are entered manually. | ||
| """ | ||
|
|
||
| @property | ||
| def table_name(self): | ||
| @classproperty | ||
| def table_name(cls): | ||
| """ | ||
| :returns: the table name of the table formatted for mysql. | ||
| """ | ||
| return from_camel_case(self.__class__.__name__) | ||
| return from_camel_case(cls.__name__) | ||
|
|
||
|
|
||
| class Lookup(BaseRelation, metaclass=abc.ABCMeta): | ||
| class Lookup(UserRelation): | ||
| """ | ||
| Inherit from this class if the table's values are for lookup. This is | ||
| currently equivalent to defining the table as Manual and serves semantic | ||
| purposes only. | ||
| """ | ||
|
|
||
| @property | ||
| def table_name(self): | ||
| @classproperty | ||
| def table_name(cls): | ||
| """ | ||
| :returns: the table name of the table formatted for mysql. | ||
| """ | ||
| return '#' + from_camel_case(self.__class__.__name__) | ||
| return '#' + from_camel_case(cls.__name__) | ||
|
|
||
| def _prepare(self): | ||
| """ | ||
|
|
@@ -64,29 +88,29 @@ def _prepare(self): | |
| self.insert(self.contents, skip_duplicates=True) | ||
|
|
||
|
|
||
| class Imported(BaseRelation, AutoPopulate, metaclass=abc.ABCMeta): | ||
| class Imported(UserRelation, AutoPopulate): | ||
| """ | ||
| Inherit from this class if the table's values are imported from external data sources. | ||
| The inherited class must at least provide the function `_make_tuples`. | ||
| """ | ||
|
|
||
| @property | ||
| def table_name(self): | ||
| @classproperty | ||
| def table_name(cls): | ||
| """ | ||
| :returns: the table name of the table formatted for mysql. | ||
| """ | ||
| return "_" + from_camel_case(self.__class__.__name__) | ||
| return "_" + from_camel_case(cls.__name__) | ||
|
|
||
|
|
||
| class Computed(BaseRelation, AutoPopulate, metaclass=abc.ABCMeta): | ||
| class Computed(UserRelation, AutoPopulate): | ||
| """ | ||
| Inherit from this class if the table's values are computed from other relations in the schema. | ||
| The inherited class must at least provide the function `_make_tuples`. | ||
| """ | ||
|
|
||
| @property | ||
| def table_name(self): | ||
| @classproperty | ||
| def table_name(cls): | ||
| """ | ||
| :returns: the table name of the table formatted for mysql. | ||
| """ | ||
| return "__" + from_camel_case(self.__class__.__name__) | ||
| return "__" + from_camel_case(cls.__name__) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why add more lines of code unncessarily? Since we are using
inspect, we should just useinspect.getmembersrather than manually traversingdirand applyingifselection.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please correct me if I am wrong, but I think
inspect.getmembersactually evaluates every member. So if there is a computed property, e.g._populated_from, it will actually trigger its computation. In contrast, the proposed implementation looks at the names first and only evaluates members if they start with a capital letter. So it avoids extra computations.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your concern. As long as you are invoking
inspect.getmemberson a class and that there is no member that implements__get__, no additional computation is performed. However, I see that you have reintroducedclasspropertyconcept, and if such thing is present, indeedinspect.getmemberswill evaluate the class property.Also, since
_populated_fromis directly set equal to a relation object, there is no real trigger of computation caused by just accessing the member, if that's what you are concerned with.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought
propertyimplemented__get__. No?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default
AutoPopulate._populated_fromis a property that loads dependencies, looks up parents, and returns the join of the parents.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
propertyand thus_populated_fromdoesn't run the function unless it's run on an instance. That's why I was saying that as long as you invokeinspect.getmemberson a class (which is what we'd do anyway), the content of the_populated_fromwill not be evaluated.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I don't feel too strongly about it. It seems that
inspectis designed for inspecting objects during debugging with no regard for performance.inspect.getmembersseems to be doing a lot more work than necessary.