Skip to content
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

Merged
merged 3 commits into from Jan 5, 2023
Merged

Conversation

jschueller
Copy link
Contributor

Closes #10944

@jschueller jschueller force-pushed the imgmath_relfn branch 2 times, most recently from 5381a22 to 3758c58 Compare November 14, 2022 07:23
@return42
Copy link

return42 commented Dec 2, 2022

@jschueller thanks a lot for the patch 👍

The patch fixed the issue for a .. math:: directive

.. math::
   :label: schroedinger general

    \mathrm{i}\hbar\dfrac{\partial}{\partial t} |\,\psi (t) \rangle =
          \hat{H} |\,\psi (t) \rangle.

But in my tests :math: inline examples are still broken::

``\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}`

@jschueller
Copy link
Contributor Author

@return42 and now ?

@return42
Copy link

return42 commented Dec 2, 2022

Perfect .. works like a charm:

grafik


IDK why the CI reports an flake8 issue not related to this patch.

@jschueller
Copy link
Contributor Author

lets hope it can be merged soon

@return42
Copy link

return42 commented Dec 2, 2022

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
Copy link
Contributor Author

maybe @marxin or @jfbu ?

@jfbu
Copy link
Contributor

jfbu commented Jan 4, 2023

@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?

Copy link
Contributor

@jfbu jfbu left a 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

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.

Copy link
Contributor

@jfbu jfbu left a 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...).

@jschueller
Copy link
Contributor Author

ok, I updated the docstring

sphinx/ext/imgmath.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jfbu jfbu left a 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.

@AA-Turner AA-Turner changed the base branch from master to 6.0.x January 5, 2023 11:50
@AA-Turner AA-Turner merged commit 222d366 into sphinx-doc:6.0.x Jan 5, 2023
@jschueller jschueller deleted the imgmath_relfn branch January 5, 2023 12:48
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

imgmath failed to resolve image path for files in nested folder
4 participants