Open jdemeyer opened 8 years ago
Dependencies: #20825, #20686
New commits:
44e80b3 | EvaluationMethods should be a new-style class |
9a1ff6e | Improve getattr_from_other_class |
744ffa6 | Test that static methods work |
f5361d9 | Improve _sage_src_lines_() |
782f140 | 20686: minor comments improvements |
dc84d88 | Remove comments about "private" attributes with two underscores |
8a4f48d | Abstract away category lookup in cdef method getattr_from_category |
2e93afe | Merge branch 'u/jdemeyer/optimize_method_lookup_from_the_categories_for_instances_of_cython_classes' of trac.sagemath.org:sage into t/20686/optimize_method_lookup_from_the_categories_for_instances_of_cython_classes |
5619a7d | 20686: simplified the logic handling the EvaluationMethods mixin class for Expression's (internally backward incompatible) |
You really should use new-style classes. Old-style classes exist only for backwards compatibility and will be gone in Python 3.
I just moved your branch from #20686 here. Feel free to rewrite history to make it depend only on #20825.
The use of cls.__dict__
seems to preclude the use of inheritance since cls.__dict__
does not do MRO lookup while dir(cls)
does.
Thanks for creating the ticket and splitting of the commit! I am fine with this depending on #20686.
Replying to @jdemeyer:
You really should use new-style classes. Old-style classes exist only for backwards compatibility and will be gone in Python 3.
Fun: from the same premises, we arrive at opposite conclusions :-)
Here is my logical chain:
In Python 3 we won't make the inheritance from object explicit: it would be redundant to write:
class XXXMethods(object):
Furthermore, for our XXXMethods classes, it does not matter whether they are old style or new style classes (they just are bags of methods). Thus it feels natural to use right away the Python 3 idiom. That's what we have been doing in all the categories.
Replying to @jdemeyer:
The use of
cls.__dict__
seems to preclude the use of inheritance sincecls.__dict__
does not do MRO lookup whiledir(cls)
does.
That's on purpose, for consistency with what we do in categories: the XXXMethods
are meant to be pure mixins / bags of methods; they are not supposed to inherit from anything.
Replying to @nthiery:
XXXMethods
are meant to be pure mixins / bags of methods;
Given that it's a class
, I would expect it to behave like a class
. If you insist that it should not behave like a class
, then at least make it explicit and invent a new metaclass BagOfMethods
which disallows inheritance for example.
they are not supposed to inherit from anything.
How is a random Sage developer supposed to know that? That's one thing that I really don't like about the category framework in general: it makes several assumptions which work fine most of the time, but can bite you badly (the automatic binding is another such one). It almost feels like a slightly different programming language (that class
is not really a Python class, it's just a dict).
Replying to @nthiery:
Furthermore, for our XXXMethods classes, it does not matter whether they are old style or new style classes
It might matter in more places than you think. There will be some porting effort needed to transition from old-style classes to new-style classes (some issues came up in #20686). It would be better to do this now to avoid unexpected issues with Python 3.
For this reason, I am very against this change:
- class EvaluationMethods(object):
+ class EvaluationMethods:
Replying to @jdemeyer:
Replying to @nthiery:
Furthermore, for our XXXMethods classes, it does not matter whether they are old style or new style classes
It might matter in more places than you think. There will be some porting effort needed to transition from old-style classes to new-style classes (some issues came up in #20686). It would be better to do this now to avoid unexpected issues with Python 3.
For this reason, I am very against this change:
- class EvaluationMethods(object): + class EvaluationMethods:
I already did tests with inheriting from object in some XXXMethods, and the category framework kept working the exact same way. I am therefore convinced there won't be anything to change for that specific aspect for Python 3.
Furthermore, the more consistent things will be across the library, the easier the porting will be.
Cheers, Nicolas
Replying to @nthiery:
I already did tests with inheriting from object in some XXXMethods, and the category framework kept working the exact same way.
So you did some limited testing now and it worked. That's a good thing, but it doesn't guarantee that it will work in all cases now and in the future. As a general principle, we should try to move Sage as close to Python 3 as possible. Python 3 has only new-style classes, so we should use new-style classes.
Furthermore, the more consistent things will be across the library, the easier the porting will be.
True, but besides the point.
And of course, this just means that we should use new-style classes everywhere in Sage.
Replying to @jdemeyer:
Given that it's a
class
, I would expect it to behave like aclass
. If you insist that it should not behave like aclass
, then at least make it explicit and invent a new metaclassBagOfMethods
which disallows inheritance for example.
Using a metaclass would mean one more piece of purely technical syntax, which is one more chance for the programmer to forget something. Still you have a good point here: the infrastructure should check that XXXMethods inherits from nothing, and raise an explanatory error message if not.
How is a random Sage developer supposed to know that? That's one thing that I really don't like about the category framework in general: it makes several assumptions which work fine most of the time, but can bite you badly (the automatic binding is another such one). It almost feels like a slightly different programming language (that
class
is not really a Python class, it's just a dict).
That's the problem with every framework: a Django programmer needs to learn some about Django, etc, either from example by looking at existing code, or by reading the documentation.
Now does Sage really need such a framework? I am convinced enough about it to have invested altogether one solid year of work into it. Which does not mean I am not completely wrong :-)
I also believe in concise syntax with minimal boilerplate. Of course the price is additional complexity for those developers like you that not only use, but also work on the infrastructure itself.
Cheers, Nicolas
Replying to @jdemeyer:
And of course, this just means that we should use new-style classes everywhere in Sage.
Everywhere, unless we have a good reason to be convinced this does not make a difference.
Would we really gain something worth the trouble by adding an explicit inheritance from object in all 292 XXXMethods classes in Sage, and then removing them after the switch to Python3, with all the risks of induced syntactical conflicts, when we know that they are treated uniformly?
There are real issues in the switch to Python 3, and I believe this is not one.
Cheers,
Replying to @nthiery:
Using a metaclass would mean one more piece of purely technical syntax, which is one more chance for the programmer to forget something. Still you have a good point here: the infrastructure should check that XXXMethods inherits from nothing, and raise an explanatory error message if not.
I'm not saying a metaclass is the right solution, it was just some proposal. That being said, you could think if there are other ways in which your "bag-of-methods" classes differ from "real" classes. Such differences might be handled cleanly by such metaclass. For example, you could use the metaclass to disable automatic binding of methods or to disallow instantiation of the class.
Replying to @nthiery:
I also believe in concise syntax with minimal boilerplate.
I know :-) but sometimes that concise syntax just hides all kinds of stuff which is better made explicit. Like I said before: if I see a class
, I expect it to behave like a class
.
The hypothetical metaclass would also serve as entry point to documentation. If I see
class ElementMethods(BagOfStuff):
....
somewhere and want to understand what it does, I could do
sage: BagOfStuff?
which will hopefully explain that this class isn't really a class, but just some syntax to define a dict.
I don't know those XXXMethods
classes well enough to say whether it's the right solution, but it's certainly something you should consider.
And this BagOfStuff
would of course be a new-style class, rendering the other discussion obsolete :-)
I know I am repeating myself, but just to clear, I think there are two possible ways:
Either you use a plain class XXXMethods
but then it should behave like a class.
Or it's something else, say class XXXMethods(BagOfStuff)
and then you can have all kinds of strange behaviour that you document in BagOfStuff?
.
Depends on #20825 Depends on #20686
CC: @nthiery @tscrim
Component: symbolics
Author: Nicolas M. Thiéry
Branch/Commit: u/nthiery/optimize_method_lookup_from_the_categories_for_instances_of_cython_classes @
5619a7d
Issue created by migration from https://trac.sagemath.org/ticket/20859