-
-
Notifications
You must be signed in to change notification settings - Fork 55
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
Reinstate pymathics doc #778
Conversation
6289259
to
3e3aba8
Compare
3e3aba8
to
e6f50d1
Compare
DRY code. docpipeline.py `-l` `--pymathics` probably works now These changes cannot get merge before Mathics3/mathics-core#778 is merged.
46d7d20
to
f86cfa9
Compare
f86cfa9
to
f885591
Compare
self.title = "Overview" | ||
files = listdir(self.doc_dir) |
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.
A programmer who writes such a long piece of code like this without a single comment and in an __init__
method, should be graduated out of programming.
4ab3e39
to
710339f
Compare
= $Failed | ||
""" | ||
|
||
name = "LoadModule" | ||
messages = { | ||
"notfound": "Python module `1` does not exist.", | ||
"notmathicslib": "Python module `1` is not a pymathics module.", | ||
"notfound": """Problem importing Python module: `1`.""", |
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 LoadModule
produces the error notfound
, it is because the module can not be found, not because there was a generic error in importing it. So, why the new message is better?
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.
There is a difference between something existing and something importing. In fact, another check could be made to see if the file or module exists and give that message. The file or module may exist, but there is invalid Python or it may import modules that are not importable or cause circular dependencies.
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, but the error we handle in importing the python module is about the existence of the library.
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.
"Exist" is a vague or misleading word. The library can exist in some sense but not work. When I see an error message or warning I generally want to know what was tried. What was tried is that we ran a Python kind of "import": and that did not work,.
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.
In Python, what we are capturing is:
>>> import amadeupmodulename
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'amadeupmodulename'
So, maybe "Python module 1
was not found in the system"
could be a more accurate message, isn't it?
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.
There is a difference here. You issued the import
from Python and not "not found" was given in response to that. So it is clear that Python import is where the problem lies. In Mathics3 though, the user a priori doesn't know that LoadModule runs a Python import
underneath. It could have loaded the module using open()
and done some magic there.
So using the words Python and import suggest what kind of thing is failing. In fact it suggests trying to run a Python "import".
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.
What about Pymathics module `1` was not found in the system
? Otherwise, it was just a suggestion. Please just keep it as you prefer.
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.
I have been thinking about this some more. I do listen to your suggestions...
I think we can get the best of both worlds by catching any Python exception and passing back the message in the exception. In those cases where Python reports "not found" the message has that (indicating this is from an import).
In fact, it was probably a mistake using except ImportError
rather then except Exception
.
@@ -689,251 +1017,32 @@ def __str__(self): | |||
return self.test | |||
|
|||
|
|||
# FIXME: think about - do we need this? Or can we use DjangoMathicsDocumentation and | |||
# LatTeXMathicsDocumentation only? |
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.
Are we using this? Maybe we could replace it with a function that produces the right class of object.
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.
Yes, we are using this. Merging the two is another mess that I don't want to deal with right now.
If you want to try to do it go head. I will note that there are still lots of opportunities for DRYing the code and improving it
The motivation here was simply that we had code in Django to handle Mathics3 Modules and I wanted that used on the LaTeX/PDF side. So moving this code to common code seemed like the way to do this which both accomplishes what I want to do while improving the code.
However in doing so there have been small breakages here and there precisely because there were two sets of code and an improvement or necessary change was made in one copy but not the other. And the one with the improvement did not make it to the common code.
Last weekend I had my fill of trying to find and fix these kinds of things. Doing more in order to DRY the code more is left for the future for me.
Apart from my comments, and the need of merging first #781 to avoid the hanging, LGTM. |
There is another thing: for some reason, the pymathics module is not loaded when the module doctests are run |
Works for me:
What do you get when you run |
This PR fixes another issue with precision. In particular, this issue was the reason of the hangings in doctest in #778.
Move code from django_doc and latex_doc .py to common_doc.py In the process we should handle pymathics better. Remove a number of unused functions and variables like ``is_private`` and pymathics_doc_loaded.
See if we can get by with out using the want_sorting parameter
Pull out flaky Unique[] doctests and start a more robust pytest for these. more work is needed.
Fix bug in InterpretedBox
Reinstate hard-coded XMLDoc where self.doc_fn() is not available Reinstante want_sorting option
--pymathics -> --load-module. Note that one splits module names with a comma. The name Pymathics is deprecated
This should make it a little more robust.
3e1f91a
to
e62333f
Compare
* DRY doc code Much has been moved to mathics.doc.mathics.common now * Simplify: Remove bogus default_pymathics_modules See if we can get by without want_sorting * Reduce custom doc code... DRY code. docpipeline.py `-l` `--pymathics` probably works now These changes cannot get merge before Mathics3/mathics-core#778 is merged. * Reinstante guide section summaries * Reinstate want_sorting option * Remove "remake" for now * want sorting parameter adjustment
This allows Mathics3 Modules loaded via
-l
or--load-module
on thedocpipetest.py
command like to get loadedusing
eval_LoadModule
the underlying eval method forLoadModule[]
.In other words, from Mathics3 core you can test Mathics3 Modules or select individual sections Mathics3 Module tests. But that also means, we are collecting XML-like documentation and test data which can then be put into the LaTeX-generated PDF.
I believe the information will go into the PCL (Python Pickle file), but I haven't tested that yet.
And I haven't looked at the piece that puts creates
documentation.tex
(mathics/doc/latex/doc2latex.py
)I think that will be another (small) PR. This is large enough as it is.
For expediency in being able to get this accomplished over the weekend in a limited time, there are/were several separable commits here. I am sure you know the pattern - you want to get X done but bug Y gets in the way. So you fix Y, and then bug Z pops up.
Over the week I will try to extract these so that they can be reviewed/discussed independently.
Here are two that I can think of though.
I would like to change the name Pymathics or Pymathics module to the simpler "Mathics3 Module" which has always been loaded via
LoadModule
.The messages when a Mathics3 Module can't be loaded have been changed.