-
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
Conversation
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 use inspect.getmembers rather than manually traversing dir and applying if selection.
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.getmembers actually 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.getmembers on a class and that there is no member that implements __get__, no additional computation is performed. However, I see that you have reintroduced classproperty concept, and if such thing is present, indeed inspect.getmembers will evaluate the class property.
Also, since _populated_from is 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 property implemented __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_from is 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 property and thus _populated_from doesn't run the function unless it's run on an instance. That's why I was saying that as long as you invoke inspect.getmembers on a class (which is what we'd do anyway), the content of the _populated_from will 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 inspect is designed for inspecting objects during debugging with no regard for performance. inspect.getmembers seems to be doing a lot more work than necessary.
Fixed issue #178