Opened 2 years ago

Closed 23 months ago

Last modified 18 months ago

#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:

Change History (24)

comment:1 Changed 2 years ago by chapoton

  • Branch set to u/chapoton/24228
  • Commit set to 1113dea74be379432cab4eafd140150d1ddbc8fe
  • Status changed from new to needs_review

New commits:

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

comment:2 Changed 2 years ago by chapoton

  • Description modified (diff)

comment:3 Changed 2 years ago by tscrim

  • Reviewers set to Travis Scrimshaw

LGTM if green patchbot.

comment:4 Changed 2 years ago by chapoton

  • Status changed from needs_review to positive_review

Thanks

comment:5 Changed 2 years ago by embray

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 2 years ago by embray

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 2 years ago by embray

  • Status changed from positive_review to needs_info

comment:8 Changed 2 years ago by embray

Please see #24258

comment:9 follow-up: Changed 2 years ago by 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.

comment:10 in reply to: ↑ 9 Changed 2 years ago by embray

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: Changed 2 years ago by tscrim

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 2 years ago by embray

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 2 years ago by embray

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 2 years ago by embray

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 2 years ago by tscrim

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: Changed 2 years ago by 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.

comment:17 follow-up: Changed 2 years ago by jdemeyer

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 2 years ago by tscrim

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 2 years ago by embray

It's really not complicated...

comment:20 in reply to: ↑ 17 Changed 2 years ago by embray

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...

comment:21 Changed 2 years ago by chapoton

  • 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:22 Changed 23 months ago by chapoton

  • Cc embray added

Erik, could you please close this one ?

comment:23 Changed 23 months ago by embray

  • Resolution set to wontfix
  • Status changed from positive_review to closed

comment:24 Changed 18 months ago by chapoton

  • Branch u/chapoton/24228 deleted
  • Commit 1113dea74be379432cab4eafd140150d1ddbc8fe deleted
Note: See TracTickets for help on using tickets.