Opened 21 months ago

Closed 19 months ago

Last modified 12 months ago

#24258 closed enhancement (fixed)

py3: support <type '...'> expected output in doctests on Python 3

Reported by: embray Owned by:
Priority: major Milestone: sage-8.2
Component: python3 Keywords:
Cc: chapoton Merged in:
Authors: Erik Bray Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 514ed93 (Commits) Commit:
Dependencies: Stopgaps:

Description

On Python 3 all classes, including built-in types, are represented as <class '...'> instead of <type '...'> as Python 2 uses for built-in types.

This normalizes the expected doctest output on Python 3 to account for this difference.

Whenever Sage switches to Python 3 as a default it would be simple to also reverse the sense of this normalization, and update the doctests to expect <class '...'>.

This replaces #24228, #24229, #24230, and #24233.

Change History (26)

comment:1 Changed 21 months ago by embray

  • Dependencies set to #24257

comment:2 Changed 21 months ago by embray

  • Status changed from new to needs_review

comment:3 in reply to: ↑ description ; follow-up: Changed 21 months ago by jdemeyer

Replying to embray:

Whenever Sage switches to Python 3 as a default it would be simple to also reverse the sense of this normalization, and update the doctests to expect <class '...'>.

Wouldn't it make sense to do that now, i.e. to replace <type by <class ? That would be more future-proof.

comment:4 in reply to: ↑ 3 ; follow-up: Changed 21 months ago by embray

Replying to jdemeyer:

Replying to embray:

Whenever Sage switches to Python 3 as a default it would be simple to also reverse the sense of this normalization, and update the doctests to expect <class '...'>.

Wouldn't it make sense to do that now, i.e. to replace <type by <class ? That would be more future-proof.

*nod* that was my original plan, but I decided:

a) This would be a less disruptive change for now--just a small change in one module vs. a small change in many

b) For the time being Python 2 is still the default for Sage, so changing all the existing tests (particularly "EXAMPLE" docs) could confuse readers.

comment:5 Changed 21 months ago by jdemeyer

  • Status changed from needs_review to needs_work

Lots of breakage on the patchbot.

comment:6 in reply to: ↑ 4 Changed 21 months ago by jdemeyer

Replying to embray:

a) This would be a less disruptive change for now--just a small change in one module vs. a small change in many

If you plan a disruptive change anyway when we switch to Python 3, that argument makes little sense. Why is a disruptive change later better than a disruptive change now.

b) For the time being Python 2 is still the default for Sage, so changing all the existing tests (particularly "EXAMPLE" docs) could confuse readers.

Fair enough.

comment:7 Changed 21 months ago by embray

I believe most of the (relevant) failures are actually due to #24257, where they've since been fixed. I can rebase this on top of that....

comment:8 Changed 21 months ago by git

  • Commit changed from b5ba2ede2997b247d88389623adde2e0524580cc to e12b67fda63bf721cf519c4553c5abef0e8553ef

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

6a9e6cbRemove 'from __future__ import unicode_literals from some modules.
1add357Add a doctest output fixup to replace <type '...'> with <class '...'> so that
e12b67fFix docstring--the normalization is currently not performed on Python 2

comment:9 follow-up: Changed 21 months ago by embray

A similar, though not exactly the same difference I've noticed is that on Python 3 unbound methods of classes (which are just normal functions) are repr'd with their __qualname__ which includes the class name. E.g.

>>> class Foo:
...     def a(self): pass
...
>>> Foo.a
<function Foo.a at 0x7fe3d04c9b20>

So some tests that happen to rely on such output fail on python 3.

The tests could be rewritten, but it might be nice to automatically account for this difference. In principle there's a little advantage to having the __qualname__ in the output since we can also also see in what class the function was defined, but in the meantime we don't lose anything over what we already have by ignoring it.

I could either add that to this ticket, or add a new one. It might be worth adding to this one since the same issue applies to nested classes.

comment:10 in reply to: ↑ 9 Changed 21 months ago by embray

Replying to embray:

I could either add that to this ticket, or add a new one. It might be worth adding to this one since the same issue applies to nested classes.

I implemented a workaround for this, but in implementation it's distinct enough from this issue that I'll just open a separate ticket for this once I'm confident that it's ready.

comment:11 Changed 20 months ago by embray

  • Status changed from needs_work to needs_review

I don't think this actually needs any new work--it's been working fine for me on my python 3 branch for some time. The previous patchbot failures were due to #24257 which is now closed.

comment:12 Changed 20 months ago by embray

Would be nice to get this done--I have a number of other test output normalizations that I've added since this one.

comment:13 Changed 20 months ago by jdemeyer

Please add a test showing how normalize_type_repr works when given a more complicated expression. Something like:

sage: L = [Integer, float]
sage: normalize_type_repr(repr(L))

comment:14 Changed 20 months ago by embray

Sure--let me make sure that actually works in the first place (it should--I think I've already encountered some examples like that)

comment:15 Changed 20 months ago by embray

  • Status changed from needs_review to needs_work

comment:16 Changed 20 months ago by git

  • Commit changed from e12b67fda63bf721cf519c4553c5abef0e8553ef to 6760db0bc5406c4c36d131dad4d928e31f806008

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

6760db0Add an additional test for normalize_type_repr

comment:17 Changed 20 months ago by embray

  • Status changed from needs_work to needs_review

comment:18 Changed 20 months ago by jdemeyer

  • Status changed from needs_review to needs_work

Patchbot complains about docbuild.

comment:19 follow-ups: Changed 20 months ago by jdemeyer

If I understand things correctly, this ticket ensures that both the following doctests would pass on Python 3:

sage: int
<class 'int'>

and

sage: int
<type 'int'>

but on Python 2 only

sage: int
<type 'int'>

would be accepted. Is that right?

I would very much prefer to accept <class 'int'> on Python 2 too. That way, we could at least make new doctests more future-proof by using <class instead of <type.

comment:20 Changed 20 months ago by chapoton

  • Branch changed from u/embray/python3/type-repr-fixups to public/python3/type-repr-fixups
  • Commit changed from 6760db0bc5406c4c36d131dad4d928e31f806008 to 514ed9369f12f760b6e714468f45b3ce6c7b20a9
  • Dependencies #24257 deleted

I fixed the doc


New commits:

476a27bMerge branch 'u/embray/python3/type-repr-fixups' in 8.2.b2
514ed93trac 24258 make doc build

comment:21 in reply to: ↑ 19 Changed 20 months ago by chapoton

Replying to jdemeyer:

If I understand things correctly, this ticket ensures that both the following doctests would pass on Python 3:

sage: int
<class 'int'>

and

sage: int
<type 'int'>

but on Python 2 only

sage: int
<type 'int'>

would be accepted. Is that right?

I would very much prefer to accept <class 'int'> on Python 2 too. That way, we could at least make new doctests more future-proof by using <class instead of <type.

We can easily make new doctests future-proof by simply using <... 'int'>. There is a patchbot plugin that checks for that.

comment:22 in reply to: ↑ 19 Changed 20 months ago by embray

  • Status changed from needs_work to positive_review

Replying to jdemeyer:

I would very much prefer to accept <class 'int'> on Python 2 too. That way, we could at least make new doctests more future-proof by using <class instead of <type.

I thought I already addressed this, but it doesn't make enormous sense to write new doctests against Python 3-style output as long as Sage's default python remains python 2--that's what the documentation should be written against. When Sage switches to python 3 as the default python, then it makes sense to make mass replacements on the documentation too, and not until then.

I wouldn't be opposed to making the output checker flexible in both cases, but I don't think there's any good reason to right now either.

comment:23 Changed 20 months ago by chapoton

  • Reviewers set to Frédéric Chapoton

comment:24 Changed 19 months ago by vbraun

  • Branch changed from public/python3/type-repr-fixups to 514ed9369f12f760b6e714468f45b3ce6c7b20a9
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:25 follow-up: Changed 12 months ago by chapoton

  • Commit 514ed9369f12f760b6e714468f45b3ce6c7b20a9 deleted

This seems not to work on

sage -t --long src/sage/rings/universal_cyclotomic_field.py  # 1 doctest failed

with sage 8.4.b4.

comment:26 in reply to: ↑ 25 Changed 12 months ago by embray

Replying to chapoton:

This seems not to work on

sage -t --long src/sage/rings/universal_cyclotomic_field.py  # 1 doctest failed

with sage 8.4.b4.

It's because the <type ...> repr is split onto multiple lines, which is not supported. You could either change the regexp to be multi-line (but this introduces more complications), or just fix that test to not split <type ...> across lines.

Note: See TracTickets for help on using tickets.