#24228 closed enhancement (wontfix)
py3: replace <type by <... (in pyx files of rings folder)
Reported by: | chapoton | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/invalid/wontfix |
Component: | python3 | Keywords: | |
Cc: | embray | Merged in: | |
Authors: | Frédéric Chapoton | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
part of #16085
Change History (24)
comment:1 Changed 3 years ago by
- Branch set to u/chapoton/24228
- Commit set to 1113dea74be379432cab4eafd140150d1ddbc8fe
- Status changed from new to needs_review
comment:2 Changed 3 years ago by
- Description modified (diff)
comment:5 Changed 3 years ago by
Personally I'm -1 on changing this en mass, purely from the perspective of documentation quality. If these were all pure tests that aren't shown in the documentation that would be one thing, but when it comes to usage examples I don't like it. It would be nicer to just display one or the other (probably <class ...>
for forward-compatibility), and handle the difference in the doctest output checker.
comment:6 Changed 3 years ago by
I'm working on an update to the output checker to be flexible about this. Would you consider putting these tickets on hold instead?
comment:7 Changed 3 years ago by
- Status changed from positive_review to needs_info
comment:8 Changed 3 years ago by
Please see #24258
comment:9 follow-up: ↓ 10 Changed 3 years ago by
I think it would cause more problems/confusion for the user to unexpectedly get something different than when they ran the test (assuming they understand ...
). Moreover, I feel that #24258 makes the conversion to Python3 harder because it suppresses the distinctions in a hidden way as opposed to the clearly hacked ...
approach. I do agree that it does make the doctests less clean. However, I have less invested than others on Python3, so perhaps they can comment.
From a quick look, a number of the tests probably could be better implemented to not create the output difference (i.e. explicitly test against the type or not have output). So we might also want to spend some effort on that rather than relying on workarounds.
comment:10 in reply to: ↑ 9 Changed 3 years ago by
Replying to tscrim:
I think it would cause more problems/confusion for the user to unexpectedly get something different than when they ran the test (assuming they understand
...
). Moreover, I feel that #24258 makes the conversion to Python3 harder because it suppresses the distinctions in a hidden way as opposed to the clearly hacked...
approach. I do agree that it does make the doctests less clean. However, I have less invested than others on Python3, so perhaps they can comment.From a quick look, a number of the tests probably could be better implemented to not create the output difference (i.e. explicitly test against the type or not have output). So we might also want to spend some effort on that rather than relying on workarounds.
This is the problem with using doctests everywhere. From a documentation standpoint looking at the repr is more interesting. It depends, to me, if the test is in an "EXAMPLES" block or a "TESTS" block. In the former case I think it should be more interesting for reading purposes.
Casual readers of the docs don't necessarily know what ...
means in this context. In many cases it's pretty clear (e.g. trailing ellipses at the end of some long floating point expansion). But here it's just awkward and ugly. For the purposes of testing, especially considering that Python 3 is currently not the default (and won't be for still quite a while) it's better to leave existing tests alone as much as possible and adapt minor repr
differences to Python 3 only.
comment:11 follow-up: ↓ 13 Changed 3 years ago by
We are in agreement that they are not pretty, but I think it is much better to show the differences between Python2 and Python3 doctests. Actually, perhaps the best solution would be the output flag suggested on #24261 along with writing better doctests in the cases we can. Thoughts?
comment:12 Changed 3 years ago by
It's better in cases that actually matter--this is just a trivial formatting difference that does not impact the validity of the relevant tests (one can invent a pathological case where it matters, such as replacing the builtin int
with a user-defined class named "int"). By putting in "..." anywhere one isn't "showing" anything, just brushing the difference under the rug, rather than handling it an explicitly and restrictively.
comment:13 in reply to: ↑ 11 Changed 3 years ago by
Also, the point of #24261 is specifically for those relatively rare cases where there is an expected non-trivial difference in behavior between Python 2 and 3. Using it for this case would mean hundreds of examples like
<type 'int'> # py2 <class 'int'> # py3
See? It's silly when you look at it that way.
comment:14 Changed 3 years ago by
I see a bunch of these got merged anyways since I didn't change the status on them. I'd really prefer to undo that.
comment:15 Changed 3 years ago by
For the record, I will not actively oppose (up to suggesting changing the appropriate doctests to remove the compatibility issue), but I will not be reviewing those tickets either.
comment:16 follow-up: ↓ 18 Changed 3 years ago by
I still don't understand your opposition here. It feels like a bikeshed. We're just talking about making a few documentation examples work more easily testable.
comment:17 follow-up: ↓ 20 Changed 3 years ago by
Whatever you do, please beware of conflicts with #24350. I already had several merge conflicts because of changing <type
to <...
.
comment:18 in reply to: ↑ 16 Changed 3 years ago by
Replying to embray:
I still don't understand your opposition here. It feels like a bikeshed. We're just talking about making a few documentation examples work more easily testable.
How I see your proposal is that we should hide much more of the difference between Python2/3 by making the framework itself more complicated (and thus more prone to bugs). So IMO it is far from bikeshedding. But like I said, I am not opposing your proposal, but instead choosing not to support it.
Really, we should have very minimal tests that honestly need to include a check that has the output type/class there. Some better (and more robust) checks would be to do isinstance(foo, Bar)
. So I doubt we would really have to have hundreds of cases as you say in comment:13. However, that might require looking a bit at the actual doctest itself, and so it is not so mechanical of a change.
comment:19 Changed 3 years ago by
It's really not complicated...
comment:20 in reply to: ↑ 17 Changed 3 years ago by
comment:21 Changed 3 years ago by
- Milestone changed from sage-8.1 to sage-duplicate/invalid/wontfix
- Status changed from needs_info to positive_review
Let us close this one as invalid for the time being, please. This will only become a serious issue when we can start to run the tests in python3.
comment:23 Changed 3 years ago by
- Resolution set to wontfix
- Status changed from positive_review to closed
comment:24 Changed 3 years ago by
- Branch u/chapoton/24228 deleted
- Commit 1113dea74be379432cab4eafd140150d1ddbc8fe deleted
New commits:
py3: replace <type 'xxx'> by <... 'xxx'> in the doctests of rings/ pyx files