py3: replace <type by <... (in pyx files of rings folder)

[fchapoton]

part of #16085

CC: @embray

Component: python3

Author: Frédéric Chapoton

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/24228

[fchapoton]

Commit: 1113dea

[fchapoton]

New commits:

1113dea

py3: replace by <... 'xxx'> in the doctests of rings/ pyx files

[fchapoton]

Branch: u/chapoton/24228

[tscrim]

comment:3

LGTM if green patchbot.

[tscrim]

Reviewer: Travis Scrimshaw

[fchapoton]

comment:4

Thanks

[embray]

comment:5

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.

[embray]

comment:6

I'm working on an update to the output checker to be flexible about this. Would you consider putting these tickets on hold instead?

[embray]

comment:8

Please see #24258

[tscrim]

comment:9

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.

[embray]

comment:10

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.

[tscrim]

comment:11

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?

[embray]

comment:12

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.

[embray]

comment:13

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.

[embray]

comment:14

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.

[tscrim]

comment:15

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.

[embray]

comment:16

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.

[jdemeyer]

comment:17

Whatever you do, please beware of conflicts with #24350. I already had several merge conflicts because of changing <type to <....

[tscrim]

comment:18

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.

[embray]

comment:19

It's really not complicated...

[embray]

comment:20

Replying to @jdemeyer:

Whatever you do, please beware of conflicts with #24350. I already had several merge conflicts because of changing <type to <....

Perhaps in that case we could revert those changes. I meant to put the other tickets for that on hold as well...

[fchapoton]

comment:21

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.

[fchapoton]

comment:22

Erik, could you please close this one ?

[fchapoton]

Changed branch from u/chapoton/24228 to none

[fchapoton]

Changed commit from 1113dea to none