Opened 4 years ago
Closed 3 years ago
#24562 closed defect (fixed)
py3: fixes to sage.misc.decorators
Reported by: | embray | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.7 |
Component: | python3 | Keywords: | |
Cc: | chapoton | Merged in: | |
Authors: | Erik Bray | Reviewers: | Frédéric Chapoton |
Report Upstream: | N/A | Work issues: | |
Branch: | 44ee2ec (Commits, GitHub, GitLab) | Commit: | 44ee2ec1bf1c6cd3e1d4654a2ad3696fe3376b2b |
Dependencies: | Stopgaps: |
Description
Change History (29)
comment:1 Changed 4 years ago by
- Commit set to e7a5e3929ef2108094549a97e63e1dba3fb9b09f
comment:2 Changed 4 years ago by
- Status changed from new to needs_review
comment:3 Changed 4 years ago by
- Status changed from needs_review to needs_info
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...
comment:4 Changed 4 years ago by
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:
comment:5 Changed 4 years ago by
- Status changed from needs_info to needs_work
The new version doesn't deal with __doc__
correctly. Try adding this patch:
-
src/sage/misc/decorators.py
diff --git a/src/sage/misc/decorators.py b/src/sage/misc/decorators.py index ab16139..62e8a6a 100644
a b class infix_operator(object): 191 191 192 192 An infix dot product operator:: 193 193 194 sage: def dot(a,b): return a.dot_product(b) 194 sage: def dot(a,b): 195 ....: "Dot product" 196 ....: return a.dot_product(b) 195 197 sage: dot=infix_operator('multiply')(dot) 196 198 sage: u=vector([1,2,3]) 197 199 sage: v=vector([5,4,3])
comment:6 Changed 4 years ago by
- Milestone changed from sage-8.2 to sage-8.3
comment:7 Changed 4 years ago by
branch does not apply anymore
comment:8 Changed 4 years ago by
- Commit changed from e7a5e3929ef2108094549a97e63e1dba3fb9b09f to 0bb9b1a7c42e21b926e8546df179d1fff4ff1380
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
0bb9b1a | Python 3 fixes for sage.misc.decorators.
|
comment:9 Changed 4 years ago by
Rebased. I still need to see about addressing Jeroen's comment.
comment:10 Changed 4 years ago by
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.
comment:11 Changed 4 years ago by
- Commit changed from 0bb9b1a7c42e21b926e8546df179d1fff4ff1380 to 1f61a71b8b899cec2a4477f171e5135540b7140f
comment:12 Changed 4 years ago by
- Commit changed from 1f61a71b8b899cec2a4477f171e5135540b7140f to 44ee2ec1bf1c6cd3e1d4654a2ad3696fe3376b2b
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
|
comment:13 Changed 4 years ago by
Added a little cleanup on the tests for infix_operator
. Perhaps should add more tests, but unique tests :)
comment:14 Changed 4 years ago by
- Status changed from needs_work to needs_review
comment:15 Changed 4 years ago by
- Reviewers set to Julian Rüth
- Status changed from needs_review to needs_work
I believe that by our policy the methods in here should have doctests. Or is there some reason why they shouldn't?
comment:16 follow-up: ↓ 19 Changed 4 years ago by
- Status changed from needs_work to needs_review
What methods do you think require tests? These are mostly internal details that are tested implicitly by the existing examples.
comment:17 Changed 4 years ago by
- Milestone changed from sage-8.3 to sage-8.4
I believe this issue can reasonably be addressed for Sage 8.4.
comment:18 Changed 4 years ago by
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.
comment:19 in reply to: ↑ 16 Changed 4 years ago by
- Status changed from needs_review to needs_work
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.
comment:20 follow-up: ↓ 22 Changed 4 years ago by
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.
comment:21 follow-up: ↓ 23 Changed 4 years ago by
- Status changed from needs_work to needs_review
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.
comment:22 in reply to: ↑ 20 Changed 4 years ago by
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__
.
comment:23 in reply to: ↑ 21 Changed 4 years ago by
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.
comment:24 Changed 4 years ago by
- Reviewers Julian Rüth deleted
comment:25 Changed 4 years ago by
- Cc chapoton added
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?
comment:26 Changed 4 years ago by
- Milestone changed from sage-8.4 to sage-8.5
comment:27 Changed 3 years ago by
- Milestone changed from sage-8.5 to sage-8.7
Retargeting some of my tickets (somewhat optimistically for now).
comment:28 Changed 3 years ago by
- Reviewers set to Frédéric Chapoton
- Status changed from needs_review to positive_review
ok. I would have prefered to see stupid doctests everywhere, but let us move on.
comment:29 Changed 3 years ago by
- Branch changed from u/embray/python3/sage-misc-decorators to 44ee2ec1bf1c6cd3e1d4654a2ad3696fe3376b2b
- Resolution set to fixed
- Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Python 3 fixes for sage.misc.decorators.