py3: fixes to sage.misc.decorators

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​e7a5e39

Python 3 fixes for sage.misc.decorators.

[jdemeyer]

As far as I can tell there's only one place in Sage that uses sage_wraps on a class at all

Where is this "one place in Sage"? If you want me to test this, I guess I should know...

[chapoton]

There is only one place..

git grep -A2 -B2 "@sage_wrap" | grep -B2 "class"
src/sage/misc/decorators.py-
src/sage/misc/decorators.py:        @sage_wraps(func)
src/sage/misc/decorators.py-        class wrapper:

[jdemeyer]

The new version doesn't deal with __doc__ correctly. Try adding this patch:

src/sage/misc/decorators.py

@@ -191 +191 @@
 
     An infix dot product operator::
 
-        sage: def dot(a,b): return a.dot_product(b)
+        sage: def dot(a,b):
+        ....:     "Dot product"
+        ....:     return a.dot_product(b)
         sage: dot=infix_operator('multiply')(dot)
         sage: u=vector([1,2,3])
         sage: v=vector([5,4,3])

[chapoton]

branch does not apply anymore

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​0bb9b1a

Python 3 fixes for sage.misc.decorators.

[embray]

Rebased. I still need to see about addressing Jeroen's comment.

[embray]

I'm thinking sage_wraps shouldn't actually work on classes (the fact that it happens to work on old-style classes was kind of a lucky accident. The original functools.wraps doesn't work on classes, and was never designed to either, and sage_wraps isn't documented to work with classes. It was just used in one case (on an old-style class) and it happened to work.

It's possible to implement wraps-like functionality for classes but also a bit trickier (unfortunately). I think that would be outside the scope of this ticket.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​19f3d11	dont' try to make sage_wraps work on classes at all
​1f61a71	for compatibility with Python 3's functools.wraps, set __wrapped__ attribute on the wrapper function

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​44ee2ec

clean up the infix_operator docstrings a bit to demonstrate actual use as a decorator

[embray]

Added a little cleanup on the tests for infix_operator. Perhaps should add more tests, but unique tests :)

[saraedum]

I believe that by our policy the methods in here should have doctests. Or is there some reason why they shouldn't?

[embray]

What methods do you think require tests? These are mostly internal details that are tested implicitly by the existing examples.

[embray]

I believe this issue can reasonably be addressed for Sage 8.4.

[embray]

The failures are in -optional: magma tests that I guess are enabled on that particular buildbot. Nothing to do with this I don't think.

[saraedum]

Replying to embray:

What methods do you think require tests? These are mostly internal details that are tested implicitly by the existing examples.

Sorry, but I don't understand what "internal details" means. I know we had this discussion before but unless something has changed, virtually all methods should be doctested, e.g., the ones in _infix_wrapper. Personally, I have no opinion on whether that's a good or a bad policy (I can see valid arguments on both sides, so I am not sure) but I think it's still the way things work; which could be changed of course.

Btw, with the current state of documentation it is not immediately clear to me what _infix_wrapper does. A docstring that explains what this class is doing would be helpful.

[embray]

It doesn't always make sense to write examples for methods that merely serve some internal purpose and are never used directly--instead the tests use these things indirectly in the process of doing the thing it is they're supposed to do. What this leads to is tests like you can see in the original code, such as:

-        def left_func(self, right):
-            """
-            The function for the operation on the left (e.g., __add__).
-
-            EXAMPLES::
-
-                sage: def dot(a,b): return a.dot_product(b)
-                sage: dot=infix_operator('multiply')(dot)
-                sage: u=vector([1,2,3])
-                sage: v=vector([5,4,3])
-                sage: u *dot* v
-                22
-            """

This exact same test is repeated like 3 or 4 times in that module, because someone said "it needs an example block" in every method. This should be clarified in the developer docs, but that really does not mean "literally every def statement". The intention is that every function or method that someone actually uses directly is documented in this way.

Sometimes it still makes sense to have as an example of how an internal method is used, if it's unclear, or if it's an internal method that might be useful when adding new methods to a class or subclass. But not if it means going into contortions to set up a test for a method that is never called directly in the first place.

[embray]

Anyways, this ticket isn't adding particularly new code; just slightly refactoring existing code that was written in an odd way that made it needlessly non-portable. Needlessly duplicated doctests are also removed.

If it's unclear, _infix_wrapper is just a base class for wrappers around a function turning that function into an infix operator. infix_operator itself is just the decorator, but _infix_wrapper is the base for the actual wrapped functions returned by the decorator.

This was already in the original code under just the name "wrapper", and it was a class defined inline in infix_operator.__call__. I just moved it out to module level because there was no-need to create new instances of that base class for every infix_operator call.

[saraedum]

Replying to embray:

[...] This exact same test is repeated like 3 or 4 times in that module, because someone said "it needs an example block" in every method. This should be clarified in the developer docs, but that really does not mean "literally every def statement". The intention is that every function or method that someone actually uses directly is documented in this way.

I don't agree. Also internal methods should be documented that way wherever possible imo.

Sometimes it still makes sense to have as an example of how an internal method is used, if it's unclear, or if it's an internal method that might be useful when adding new methods to a class or subclass. But not if it means going into contortions to set up a test for a method that is never called directly in the first place.

When I'm not lazy, I try to do an "indirect doctest" when it's too much code to setup the situation where the method could be used or when the method is cleary just a backing for something else, e.g., I'd test _add_ with a+b even though it that calls __add__.

[saraedum]

Replying to embray:

Anyways, this ticket isn't adding particularly new code; just slightly refactoring existing code that was written in an odd way that made it needlessly non-portable. Needlessly duplicated doctests are also removed.

Sure, the doctests were bad. I think we should have tried to replace them with better doctests instead of no doctests.

If it's unclear, _infix_wrapper is just a base class for wrappers around a function turning that function into an infix operator. infix_operator itself is just the decorator, but _infix_wrapper is the base for the actual wrapped functions returned by the decorator.

Ok. This wasn't clear to me. That would almost be a valid docstring for the class.

So, I'll stop bugging you here I think as this probably won't lead anywhere. I am afraid I can't set this to positive review as is. If somebody else wants to do that I am not opposed at all.

[embray]

I'm not sure why you can't. Do you have some concrete suggestion for how this could be documented better, other than adding pointless repetitive doctests?

[embray]

Retargeting some of my tickets (somewhat optimistically for now).

[chapoton]

ok. I would have prefered to see stupid doctests everywhere, but let us move on.