-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
imgmath: Fix relative file path #10965
Conversation
5381a22
to
3758c58
Compare
@jschueller thanks a lot for the patch 👍 The patch fixed the issue for a .. math::
:label: schroedinger general
\mathrm{i}\hbar\dfrac{\partial}{\partial t} |\,\psi (t) \rangle =
\hat{H} |\,\psi (t) \rangle. But in my tests ``\tfrac`` **inline example** :math:`\tfrac{\tfrac{1}{x}+\tfrac{1}{y}}{y-z}`
``\dfrac`` **inline example** :math:`\dfrac{\dfrac{1}{x}+\dfrac{1}{y}}{y-z}` |
3758c58
to
756d7aa
Compare
@return42 and now ? |
Perfect .. works like a charm: IDK why the CI reports an flake8 issue not related to this patch. |
lets hope it can be merged soon |
Yes, I hope the maintainers pay attention to this bugfix. BTW: the flake8 issues reported by CI in build of master branch are known and here is the patch |
@jschueller I rebased your work on current master tip so that we have fresh linting. @AA-Turner Should we consider fixing the #10944 as done here for 6.0.1 rather than 6.1.0? |
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 reviewing this I realized #10888 modified the returned path from render_math()
to be absolute rather than relative. But the docstring still says
Line 212 in a9b0f27
Return the filename relative to the built document and the "depth", |
Could the #10888 have been achieved while keeping the behaviour of
render_math()
as formerly? And at same time avoid #10944? If not the docstring of render_math()
should be updated.
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.
Actually #10888 also modified the returned type of render_math()
but did not update accordingly the docstring, which still says that it also returns the temporary filepath as well as the absolute path of destination image file. I see no mention of this change in CHANGES or in our document. I think it is important that it should be documented somewhere (I don't know to what extent or if at all render_math()
is public API. Edit: surely not public API, but my point is that docstrings should be kept up-to-date with code...).
ok, I updated the docstring |
a6b28ef
to
3df0b86
Compare
3df0b86
to
3de5e2c
Compare
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.
LGTM, thanks
ping @AA-Turner about perhaps adding some brief summary of overall architecture at start of module as the #10888 discussion was somewhat terse and it is hard to follow it a posteriori. In particular at 4cb857d the extension internal dynamics were somewhat refactored and it would be nice to have some brief overview in plain terms of what this all does.
3de5e2c
to
0a34341
Compare
Closes #10944