-
-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Converted autogen doctests to Python syntax #26
Conversation
tests/rundoctest.py
Outdated
import doctest | ||
|
||
path = os.path.dirname(__file__) | ||
if path: | ||
os.chdir(path) | ||
os.chdir('..') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one particularly bugs me. Why were we chdir'ing before?
autogen is not a valid Python module (you would have to modify the Python path). I do not know how to properly use doctest for it. Are you able to run the doctests? |
All the doctests pass on my machine (both Py2 and Py3). I'm just trying to convince Travis to run them. I tried modifying |
I will try on mine... |
I got
which I believe is normal since autogen is not a Python module available from the Python path... |
Finally! Test pass. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is my only objection.
tests/rundoctest.py
Outdated
@@ -8,12 +8,18 @@ | |||
path = os.path.dirname(__file__) | |||
if path: | |||
os.chdir(path) | |||
os.chdir('..') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is dangerous! It will change the behavior of import cypari2
... you would better first test cypari2 as it used to be and in a second time, go to the appropriate directory and doctest autogen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would it change the behavior of import cypari2
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because you have a cypari2 repository in ../
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current version of rundoctest is doctesting the installed cypari2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.chdir
does not change the python module search path, and the search path for a script run with python script.py
does not contain .
.
The incriminated line would rather be the following one, that adds the cypari2 directory to sys.path
. But it only adds that to the end of sys.path
, so it does not take precedence over the installed cypari2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
autogen.generator
needs to read cypari2/paridecl.pxd
and cypari2/declinl.pxi
in order to run doctests, so it fails if we do not chdir.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I run python test/rundoctest.py
in a freshly cloned repo without installing I get
Traceback (most recent call last):
File "tests/rundoctest.py", line 5, in <module>
import cypari2
ModuleNotFoundError: No module named 'cypari2'
Again, that's because .
is not in sys.path
if you run python script.py
.
Isn't that enough for an error message? Why would you want a tailored message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok right. The message is good enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But you should at least replace ..
by os.path.pardir
that is more robust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
I would like
to be replaced by
|
Done. If you're happy with it, feel free to merge. |
Item 3 in #4