#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, GitHub, GitLab) | 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 '...'>
.
Change History (26)
comment:1 Changed 5 years ago by
- Dependencies set to #24257
comment:2 Changed 5 years ago by
- Status changed from new to needs_review
comment:3 in reply to: ↑ description ; follow-up: ↓ 4 Changed 4 years ago by
comment:4 in reply to: ↑ 3 ; follow-up: ↓ 6 Changed 4 years ago by
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 4 years ago by
- Status changed from needs_review to needs_work
Lots of breakage on the patchbot.
comment:6 in reply to: ↑ 4 Changed 4 years ago by
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 4 years ago by
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 4 years ago by
- Commit changed from b5ba2ede2997b247d88389623adde2e0524580cc to e12b67fda63bf721cf519c4553c5abef0e8553ef
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
6a9e6cb | Remove 'from __future__ import unicode_literals from some modules.
|
1add357 | Add a doctest output fixup to replace <type '...'> with <class '...'> so that
|
e12b67f | Fix docstring--the normalization is currently not performed on Python 2
|
comment:9 follow-up: ↓ 10 Changed 4 years ago by
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 4 years ago by
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 4 years ago by
- 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 4 years ago by
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 4 years ago by
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 4 years ago by
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 4 years ago by
- Status changed from needs_review to needs_work
comment:16 Changed 4 years ago by
- Commit changed from e12b67fda63bf721cf519c4553c5abef0e8553ef to 6760db0bc5406c4c36d131dad4d928e31f806008
Branch pushed to git repo; I updated commit sha1. New commits:
6760db0 | Add an additional test for normalize_type_repr
|
comment:17 Changed 4 years ago by
- Status changed from needs_work to needs_review
comment:18 Changed 4 years ago by
- Status changed from needs_review to needs_work
Patchbot complains about docbuild.
comment:19 follow-ups: ↓ 21 ↓ 22 Changed 4 years ago by
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 4 years ago by
- Branch changed from u/embray/python3/type-repr-fixups to public/python3/type-repr-fixups
- Commit changed from 6760db0bc5406c4c36d131dad4d928e31f806008 to 514ed9369f12f760b6e714468f45b3ce6c7b20a9
- Dependencies #24257 deleted
comment:21 in reply to: ↑ 19 Changed 4 years ago by
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 4 years ago by
- 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 4 years ago by
- Reviewers set to Frédéric Chapoton
comment:24 Changed 4 years ago by
- 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: ↓ 26 Changed 4 years ago by
- 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 4 years ago by
Replying to chapoton:
This seems not to work on
sage -t --long src/sage/rings/universal_cyclotomic_field.py # 1 doctest failedwith 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.
Replying to embray:
Wouldn't it make sense to do that now, i.e. to replace
<type
by<class
? That would be more future-proof.