-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Check modification times of included RST files #20374
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
Check modification times of included RST files #20374
Conversation
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.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers
or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
Thank you for your work on this! The approach looks sound to me, however I have a slight concern about the implementation. How "public" are these functions? That is, do we expect anyone outside of the internal code to the plot extension to be using them? These changes introduce backwards-incompatible changes (because it adds positional arguement someplace in the middle). It would be safer from a back-compat stand point (but less great from a forward looking API point of view) to do the signatures as def out_of_date(original, derived, includes=None):
if includes is None:
includes = []
... def render_figures(code, code_path
 output_dir, output_base, context,
 function_name, config, context_reset=False, 
close_figs=False, code_includes=None):
if code_includes is None:
code_includes = []
... so if anyone is using these we won't break them! |
@tacaswell I didn't think about that possibility, to be honest. I assumed that the implementation of plot_directive was internal and that the user-facing part would be the Sphinx extension itself. However, I entirely agree with the backwards compatibility point, those functions are exposed and can be imported, so there might be someone out there using it. I'll modify it as per your suggestion. Thanks for taking a look & commenting! 😄 |
@tacaswell I updated and rebased, let me know if there's anything else I could change. |
I'm not sure if it does check this for non-included files anyway, but can you update the test for this? |
@QuLogic As far as I could tell, Got a bit of a busy time ahead, so I'm not sure I'll be able to add new tests for this right now, sorry—I wouldn't mind giving it a shot in the near-ish future though! |
Co-authored with @QuLogic Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Thanks @QuLogic for your corrections! I think that the |
I just meant it could be inlined. |
Thanks @yongrenjie! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again. |
Thank you! 😄 BTW I see what you meant now with inlining... I could put in another PR if you like. def out_of_date(original, derived, includes=None):
if not os.path.exists(derived):
return True
else:
derived_mtime = os.stat(derived).st_mtime # or maybe try/except
if includes is None:
includes = []
files_to_check = [original, *includes]
return any((derived_mtime < os.stat(f).st_mtime) for f in files_to_check) |
Sure, feel free to do so. |
Fixes #17860. Briefly, to reproduce this bug:
Create a file
included.rst
with a plot inside it.Create a file
index.rst
containing the include directive.. include:: included.rst
.Build Sphinx documentation.
Modify the plotting code inside
included.rst
, but don't touchindex.rst
.Rebuild the Sphinx documentation. Because
index.rst
wasn't touched,plot_directive thinks that it's up-to-date and thus the image doesn't
need to be re-created.
This commit fixes the issue by checking the modification times not only
of the source file (
index.rst
in this case), but also of the file(s)included within it (
included.rst
).PR Summary
The commit message contains most of the info, but I attach some extra detail here just in case people come across it and find it useful. There are a couple of ways to get the included files:
Method 1
Since docutils 0.17, there is an attribute
state.document.include_log
which is a list of tuples containing the names of any files that were included, which is exactly what we want.Method 2
Directly inspect
state_machine.input_lines.items
, which is a list of tuples(source, offset)
. If a RST file is included, then we can get its filename (relative to the top-level Sphinx directory) insource
. For example, this is whatplot_directive
can when I place.. include:: included.rst
inside the file~/test/plot_dir/index.rst
:The issue with this is that there's the 'internal padding after...' line which we would have to filter out, and I'm not sure what other directives (if any) might also add some junk to this list. I got around this by first removing duplicates and then checking which of the entries were valid files.
I feel like Method 2 is a bit fragile, so I made it as a fallback:
I've tested this all the way back to docutils 0.14 and it seems to be pretty reliable.
PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).