#26314 closed enhancement (fixed)
py3: Fix interfaces module for python3
Reported by: | vklein | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.5 |
Component: | python3 | Keywords: | |
Cc: | embray, jdemeyer | Merged in: | |
Authors: | Vincent Klein | Reviewers: | Frédéric Chapoton |
Report Upstream: | N/A | Work issues: | |
Branch: | 4417bbb (Commits, GitHub, GitLab) | Commit: | 4417bbba102625f78452f6389e56b322e6abcf80 |
Dependencies: | Stopgaps: |
Description (last modified by )
There is still two remaining problems :
- I temporary used
sleep(float(0.5))
to fix a doctest but it can't be a good solution. The problem is usingsleep(0.5)
doesn't pause the thread 500 ms with python3 :
sage: time sleep(0.5) CPU times: user 48 µs, sys: 0 ns, total: 48 µs Wall time: 52.2 µs sage: time sleep(1) CPU times: user 468 µs, sys: 8 µs, total: 476 µs Wall time: 1 s
Does somebody knows if there is a ticket related to this problem or if i should open a new one ?
- In pexpect.py the doctest
sage: w = walltime(t); w > 0.4 and w < 10 True
fails in python3 because the walltime is shorter than 0.4. I don't know why it should be considered as an error or if it is expected with python3 for some reason.
Change History (22)
comment:1 Changed 4 years ago by
- Branch set to u/vklein/26314
comment:2 Changed 4 years ago by
- Commit set to 91bdf2a370a018c54dfd9762f6263f8147f938e9
- Description modified (diff)
- Status changed from new to needs_info
comment:3 Changed 4 years ago by
- Summary changed from py3: Fix interfaces modules for python3 to py3: Fix interfaces module for python3
comment:4 Changed 4 years ago by
see #26311 for the sleep problem
comment:5 Changed 4 years ago by
Thanks !
comment:6 Changed 4 years ago by
- Status changed from needs_info to needs_review
comment:7 Changed 4 years ago by
This ticket don't manage the walltime case.
comment:8 Changed 4 years ago by
- Commit changed from 91bdf2a370a018c54dfd9762f6263f8147f938e9 to 8f35e935943e0081980d8e07656fe2896acdcb1d
Branch pushed to git repo; I updated commit sha1. New commits:
8f35e93 | Trac #26314: Remove some unused import and variable.
|
comment:9 Changed 4 years ago by
Remove some unused variable to make pyflakes more happy (AsciiArtString
is needed by macaulay2.py
).
comment:10 Changed 4 years ago by
- Cc embray jdemeyer added
@embray and @jdemeyer, any opinion on the simple changes proposed here ?
comment:11 Changed 4 years ago by
... or an opinion about the "walltime" test ...
comment:12 follow-up: ↓ 13 Changed 4 years ago by
For cases like:
-
src/sage/interfaces/sagespawn.pyx
diff --git a/src/sage/interfaces/sagespawn.pyx b/src/sage/interfaces/sagespawn.pyx index 9b44d3a..1600d70 100644
a b class SageSpawn(spawn): 145 145 sage: from sage.interfaces.sagespawn import SageSpawn 146 146 sage: E = SageSpawn("sh", ["-c", "echo hello world"]) 147 147 sage: _ = E.expect_peek("w") 148 sage: E.read() 148 sage: E.read() # py2 149 149 'hello world\r\n' 150 sage: E.read() # py3 151 b'hello world\r\n'
where Python 3 returns bytes, instead of making two separate cases, in the past I have done something like print(E.read().decode('ascii'))
. This will have the same result in both cases.
I wonder if Sage shouldn't include its own sleep()
built-in that isn't broken :/
comment:13 in reply to: ↑ 12 Changed 4 years ago by
Replying to embray:
where Python 3 returns bytes, instead of making two separate cases, in the past I have done something like
print(E.read().decode('ascii'))
. This will have the same result in both cases.
I know, i hesitated between the two syntax and, as it is an EXAMPLES::
, have think that the # py2
-# py3
syntax might be clearer for the end-user.
I don't have a strong opinion on it, tell me what you prefer. If the past cases you talk about are also EXAMPLES::
we should stick with the decode('ascii')
solution for consistency.
comment:14 Changed 4 years ago by
Yes, I think consistency is best. There are a few other cases that also, rather than relying on the repr
, just do like
sage: E.read() == b'Hello\r\n' True
or whatever. This will also work the same on Python 2 and 3.
I have thought of adding a helper to the test framework to deal with b
-prefixed strings as it does with u
-strings, but it turns out they are relatively uncommon in the majority of the test suite.
comment:15 Changed 4 years ago by
- Commit changed from 8f35e935943e0081980d8e07656fe2896acdcb1d to 4417bbba102625f78452f6389e56b322e6abcf80
Branch pushed to git repo; I updated commit sha1. New commits:
4417bbb | Trac #26314 : replace '# py2 - # py3' with ...
|
comment:16 Changed 4 years ago by
# py2
-# py3
cases replaced using decode(ascii)
syntax.
comment:17 follow-up: ↓ 18 Changed 4 years ago by
probably you do not need the initial u
that you add to some doctests.
comment:18 in reply to: ↑ 17 Changed 4 years ago by
Replying to chapoton:
probably you do not need the initial
u
that you add to some doctests.
I think it is needed because on Python 2 .decode(...)
will return a unicode string literal, and the doctest parser does treat this as relevant on Python 2. The only thing it does w.r.t. u-prefixes is on Python 3 it strips them from the expected output.
I usually avoid the issue by using print(...)
instead, but i think this is fine as-is.
comment:19 Changed 4 years ago by
It is as Eric says, these tests as they are won't work on py2 without the initial u
comment:20 Changed 4 years ago by
- Reviewers set to Frédéric Chapoton
- Status changed from needs_review to positive_review
ok, let it be
comment:21 Changed 4 years ago by
- Branch changed from u/vklein/26314 to 4417bbba102625f78452f6389e56b322e6abcf80
- Resolution set to fixed
- Status changed from positive_review to closed
comment:22 Changed 4 years ago by
- Milestone changed from sage-8.4 to sage-8.5
This should be re-targeted for 8.5.
New commits:
Trac #26314 : module interface, fix code and doctest for...