Opened 4 years ago
Closed 4 years ago
#24559 closed enhancement (fixed)
py3: better backwards-compatibility for Python longs
Reported by: | embray | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.3 |
Component: | python3 | Keywords: | |
Cc: | Merged in: | ||
Authors: | Erik Bray | Reviewers: | Jeroen Demeyer |
Report Upstream: | N/A | Work issues: | |
Branch: | e3b8148 (Commits, GitHub, GitLab) | Commit: | e3b8148c764377ac86fa532432676ca357e3f56b |
Dependencies: | Stopgaps: |
Description
This does three little things to better support long
-related functionality, especially in the doctests. This allows a lot of tests to pass on Python 3 that wouldn't otherwise, or would need to be skipped and/or have Python 3 specific copies:
- The Sage preparser supports Python 2 style long literals, like
42L
. These are just treated as normal ints on Python 3 (we could also consider adding a deprecation warning for this usage). I actually wish Python itself had done this (just as they restored backwards-compat foru''
literals--that they didn't I think is just that explicitlong
s is relatively uncommon.
- The doctest parser has a fixup for recognizing long literals in the expected output of a test, and treating them as normal ints on Python 3. This is probably the weakest part of the patch in terms of implementation--it's very naïve and I can imagine the possibility of false positives, though it is still pretty unlikely. Open to better ideas on this.
- In
sage.all
I added, on Python 3 only, a new global calledlong
which is just a function that wrapsint()
and (except in the tests, for now) raises aDeprecationWarning
for usinglong()
. This still allows a lot of tests to work. I deliberately made it a simple function, and not a class, as the latter would tempt too many errors with code likeisinstance(x, long)
.
Change History (30)
comment:1 Changed 4 years ago by
- Dependencies set to #24258
- Status changed from new to needs_review
comment:2 Changed 4 years ago by
- Commit changed from 55f7a642ae60f371eed22c4aa4550cc8a30ecb78 to e3c14f32ac26b184520ee530b3e4de754971a004
comment:3 follow-up: ↓ 4 Changed 4 years ago by
I don't really with the condition deprecation depending on DOCTEST_MODE
. Doctests should be as close as possible to a normal interactive session. I'm fine with the long = int
alias, but then I would remove the deprecation.
comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 4 years ago by
Replying to jdemeyer:
I don't really with the condition deprecation depending on
DOCTEST_MODE
. Doctests should be as close as possible to a normal interactive session. I'm fine with thelong = int
alias, but then I would remove the deprecation.
This was added very deliberately in part to help with porting the doctests, so as to allow the great many doctests that use long()
to continue working for now without interruption (while at the same time giving a useful warning for user code).
My intention, though not explicitly stated, was to later (once we want to switch Sage to Python 3) remove the condition so that it's easy to find and fix/remove such tests later.
comment:5 in reply to: ↑ 4 Changed 4 years ago by
Replying to embray:
My intention, though not explicitly stated, was to later (once we want to switch Sage to Python 3) remove the condition so that it's easy to find and fix/remove such tests later.
I think it's better to just enable the alias now without any deprecation and then later add an unconditional deprecation. Since Sage doesn't really support Python 3 for the moment, the deprecation won't have much effect anyway.
comment:6 Changed 4 years ago by
Why do you think that's better? Why is this wrong?
comment:7 follow-up: ↓ 8 Changed 4 years ago by
There's a difference between "supporting Python 3" and "having Python 3 as the default Sage". I think in some version--hopefully before long (I'm running pretty low on obvious errors to fix, and most of the problems I'm seeing are minor issues in doctests) there will be a Sage that supports Python 3.
For backwards compatibility we'll want to keep Python 2 as the default for some time, but I would definitely encourage users to move to Python 3 sooner rather than later. To that end, I envision that soon Sage will have Python 2 and Python 3 binary releases, with some emphasis on getting people to use the latter. To that end it will be good to help as much as possible with transitioning.
That said, there's also a question of when to switch the default in the docs over. If the docs are Python 3 then I think there should be no long()
without a deprecation warning.
In other words, I'd be willing to change it, but it depends a lot on bigger plans regarding how Sage will transition to Python 3 that aren't particularly settled. But I feel right now that this currently with an eye toward being as helpful as possible for transitioning.
comment:8 in reply to: ↑ 7 Changed 4 years ago by
Replying to embray:
If the docs are Python 3 then I think there should be no
long()
without a deprecation warning.
Then it should be the same in doctests too. I'm fine with a deprecation or no deprecation either way, but I am protesting the condition on DOCTEST_MODE
.
comment:9 Changed 4 years ago by
I don't know why. It seems to me the whole point of having DOCTEST_MODE
flag is to control behavior in the context of doctests. Obviously this should be used sparingly, but one can always find special cases.
That said, I took a step back from this and rethought it a bit. I thought of adding a long()
in sage.all
as sort of a helper for transition to Python 3. That said, the lack of long()
in Python 3 is hardly the biggest difference, and users aren't often in the habit of using the long
built-in very much anyways (especially not in Sage where it's almost entirely unnecessary).
So maybe adding this helper isn't that useful anyways.
So what if we only injected it into the globals when running the doctests, just temporarily, no deprecation warning or anything. Later, when we want to start moving the tests to be more Python 3-compatible, we just remove it and get NameErrors? everywhere the tests try to use it.
comment:10 Changed 4 years ago by
OK, I'll stop fighting here. I don't really agree, but if you want to add long
on Python 3 if DOCTEST_MODE
without deprecation, please go ahead.
comment:11 follow-up: ↓ 12 Changed 4 years ago by
Thanks--though what do you think about my other proposal of just injecting long
for the purpose of the doctests only? I'm personally leaning toward it as a preference.
I understand your feeling that the doctests should reflect what a real user sees. The problem with Sage is that the only tests (with a few exceptions) are the doctests. And for the purposes of porting to Python 3 I don't think that philosophy really applies. It's more important just that the tests are passing so that we can narrow down on real bugs (until we make Python 3 the default, in which case yes we don't want to special case anything in the doctests if possible).
comment:12 in reply to: ↑ 11 Changed 4 years ago by
Replying to embray:
Thanks--though what do you think about my other proposal of just injecting
long
for the purpose of the doctests only? I'm personally leaning toward it as a preference.
Go for it.
comment:13 Changed 4 years ago by
- Dependencies #24258 deleted
- Status changed from needs_review to needs_work
comment:14 Changed 4 years ago by
- Status changed from needs_work to needs_review
Why needs work?
Th recent patchbot failures on this ticket are all a joke--they're broken patchbots. This one passed with the current branch on this ticket, and isn't a broken patchbot: https://patchbot.sagemath.org/log/24559/Ubuntu/16.04/x86_64/3.13.0-123-generic/sagemath-patchbot-docker/2018-02-04%2014:11:23
comment:15 Changed 4 years ago by
- Status changed from needs_review to needs_work
Ignore me--I did do the proposed work, but it seems I haven't pushed it to the upstream branch yet...
comment:16 Changed 4 years ago by
- Commit changed from e3c14f32ac26b184520ee530b3e4de754971a004 to 036a788b50a1b538c8fbd5bdb5a4ca7d07ba4a2e
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
12ff3be | Fix various minor encoding issues
|
bf6cf97 | Correct some encoding issues on Python 2.
|
4b92b47 | A few other miscellaneous fixes to the doctests tests
|
ed24910 | Various fixes intended to ensure that opened files are closed explictly
|
47e464d | Use the restore_atexit context manager for this code to work on Python 3,
|
8995cce | On Python 3, allow Python 2-style long literals (they are just treated as normal int literals)
|
3a6d2fc | Doctest normalization for Python 2 long literals.
|
22c3544 | On Python 3 add a 'long' built-in to Sage that acts as a replacement for the Python 2 'long' built-in at least for type conversions.
|
45450f3 | Properly handle cases like '1rL'
|
036a788 | py3: instead of providing a user-visible 'long' alias, just inject it into the globals for doctests, just as a transitional measure for the doctests only
|
comment:17 Changed 4 years ago by
- Dependencies set to #24343
- Status changed from needs_work to needs_review
This depends on #24343, but the new approach to handling long(...)
can be see in the latest commit.
Now, instead of providing a user-visible replacement for long
(which as I've already remarked is probably not that useful anyways, or at least a slippery-slope to providing other replacements for old Python 2 builtins), it just injects it into the globals for doctests. This should be seen purely as a transitional measure.
036a788 | py3: instead of providing a user-visible 'long' alias, just inject it into the globals for doctests, just as a transitional measure for the doctests only
|
comment:18 Changed 4 years ago by
- Dependencies changed from #24343 to #24343, #24922
comment:19 Changed 4 years ago by
- Branch changed from u/embray/python3/long-literals to u/jdemeyer/python3/long-literals
comment:20 Changed 4 years ago by
- Commit changed from 036a788b50a1b538c8fbd5bdb5a4ca7d07ba4a2e to 875149800aaa3d0660cf0d7c3789da0542748611
Rebased
New commits:
af86e5c | Various fixes intended to ensure that opened files are closed explictly where
|
436ed26 | A few other miscellaneous fixes to the doctests tests
|
e566901 | Use the restore_atexit context manager for this code to work on Python 3,
|
6fd2912 | Add run option to restore_atexit context
|
46017b6 | Use restore_atexit(run=True) in doctest framework
|
13f433c | make this exception message more flexible to the exact point in the Python interpreter where it occurred
|
4a60b3d | Merge commit '436ed266f954ecf03f894b9c6539cca353a68ad6'; commit '13f433cbfd91cd09d2415305b3c54859ef1e983c' into HEAD
|
9b24fb6 | On Python 3, allow Python 2-style long literals (they are just treated as normal int literals)
|
9178517 | Doctest normalization for Python 2 long literals.
|
8751498 | py3: instead of providing a user-visible 'long' alias, just inject it into the globals for doctests, just as a transitional measure for the doctests only
|
comment:21 Changed 4 years ago by
Jeroen, I see you rebased this, but does that constitute a "review"?
IIRC, the only open question on this ticket had to do with my plan for aliasing long
in Python 3. Originally I was adding an alias in the global namespace for all users (that would raise a deprecation warning). But now I've gone back on that, and am only injecting long
as an alias for int
into the globals for doctests.
My experience with testing this, so far, has been that it allows a lot of tests to pass on Python 3 that wouldn't otherwise, with few exceptions, and that it's a decent transitional approach.
The extra_globals
dict I added could prove useful for a few other things too.
comment:22 Changed 4 years ago by
- Commit changed from 875149800aaa3d0660cf0d7c3789da0542748611 to e3b8148c764377ac86fa532432676ca357e3f56b
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
8cbe1b3 | On Python 3, allow Python 2-style long literals (they are just treated as normal int literals)
|
5f01333 | Doctest normalization for Python 2 long literals.
|
e3b8148 | py3: instead of providing a user-visible 'long' alias, just inject it into the globals for doctests, just as a transitional measure for the doctests only
|
comment:23 Changed 4 years ago by
- Dependencies #24343, #24922 deleted
comment:24 Changed 4 years ago by
I don't like this syntax:
""" Extra objects to place in the global namespace in which tests are run. Normally this should be empty but there are special cases where it may be useful. In particular, on Python 3 add ``long`` as an alias for ``int`` so that tests that use the ``long`` built-in (of which there are many) still pass. We do this so that the test suite can run on Python 3 while Python 2 is still the default. """
It looks too much like a docstring but it isn't. Use a # comment
instead.
comment:25 follow-up: ↓ 26 Changed 4 years ago by
That's standard syntax in Sphinx for documenting class attributes.
comment:26 in reply to: ↑ 25 Changed 4 years ago by
Replying to embray:
That's standard syntax in Sphinx for documenting class attributes.
According to the Sphinx documentation, Sphinx autodoc also supports the special comment syntax #:
for this. And in fact, Sphinx itself uses that syntax.
You may think that there is no harm in using strings-which-are-not-docstrings like that, but the problem is that it teaches a bad habit. In particular the following does not contain a doctest despite looking like it does:
class X(object): attr = "something" """ EXAMPLES:: sage: 1 + 1 3 """
comment:27 Changed 4 years ago by
- Reviewers set to Jeroen Demeyer
- Status changed from needs_review to positive_review
comment:28 Changed 4 years ago by
I don't think it's a bad habit. It's perfectly acceptable syntax and I think preferable from the Sphinx perspective over the special comment syntax. If there were a notion in the Python language of attaching a docstring to a particular class attribute that's probably how I'd write it (unfortunately the concept doesn't quite fit in with how the language works, though personally I can think of ways Python might use it).
I think your concern is more a peculiarity of Sage, which overemphasizes doctests in the first place (or, perhaps if this really a problem, the doctest parser should be modified to look in triple-quoted strings anywhere in a class or module body--I think that might make sense anyways).
comment:29 Changed 4 years ago by
- Milestone changed from sage-8.2 to sage-8.3
comment:30 Changed 4 years ago by
- Branch changed from u/jdemeyer/python3/long-literals to e3b8148c764377ac86fa532432676ca357e3f56b
- Resolution set to fixed
- Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Properly handle cases like '1rL'