[MRG] Handle nested structures#904
Conversation
|
I incremented I don't really understand what these tests do, could someone please help me? 🙂 |
It ensures that only things are rebuilt that are changed. I have not looked at your code, but if your PR does something like generate an output file, it should effectively only do this if it needs to, i.e., if it has changed. In practice we currently take care of this by using a system based on MD5 hashes: 1) create the new potential file with a |
My PR indeed creates new files: it creates one new |
Take a look at how it's done elsewhere, e.g.: https://github.com/sphinx-gallery/sphinx-gallery/blob/master/sphinx_gallery/gen_rst.py#L987-L989 But IIRC it's the same name as the old just plus the extension. So if you want to create an |
|
It is not clear to me why two tests are failing.
but I can't access the log file. |
Scroll up in the output -- we treat warnings as errors, and there are a lot of warnings: You should be able to replicate locally in the |
|
I think this PR is ready for review now. Note that I tried the implementation from this PR with subsubsections, but the tests won't pass easily, so I didn't investigate any further. |
|
@lucyleeow can you look? |
|
I think I'll have time in a week or two (sorry very busy atm) |
|
Looks good but I don't see any added test that makes sure things actually work, just modified existing tests to make sure nothing breaks. The easiest way to add a test is to modify |
…o feature/nested_sections
|
Sorry it took me some time to get back to this :) I gave it some thoughts these last few days, and I couldn't think of a nice test to add (we would need new tests if we handled sub-subsections, but this PR doesn't). Besides, I would advocate that changing the way the navigation is built would be the way to go (other people think it should reflect the site's structure and they convinced me too haha). See before / after here: IMHO, this PR doesn't need more tests ; let me know what you think! |
Okay great, that's enough for me then! However, I do get new warnings on this PR when building the MNE-Python docs: From the first three warnings, it looks like some documents are created that maybe shouldn't be (?), and then with the last warning, there is a problem where I see something similar building MNE-NIRS: Thoughts on those? |
|
Thank you for testing! Sorry it took me so long to get back to you, I was busy lately 🙂 Here are some thoughts:
|
…eature/nested_sections
|
It seems that tests are failing because matplotlib changed their intersphinx references' names, so :mod: |
|
Feel free to update those here or in another PR if you want. If it's not clear what needs to be done, I could make a quick PR |
|
I gave it a quick look today but couldn't fix it right away, so I'll gladly accept your offer to make a quick PR 😄 |
I pulled your latest changes, and now I get: The contents of those two files are: Built from tutorials/README.rst: auto_tutorials/index.rstBuilt from tutorials/io/README.rst: auto_tutorials/io/index.rst |
|
Oops, I pushed a little bit too quickly, it should be better now and solve the warnings you mentioned previously. |
|
Ready for review/merge from your end @alexisthual ? |
|
MNE-Python docs: Look so much better on this PR: I'm going to assume this is good to go despite the WIP title. Thanks for this great enhancement @alexisthual ! |
|
@alexisthual any idea why this is happening on a Sphinx windows build? |
| target_dir = os.path.join(gallery_dir_abs_path, subsection) | ||
| subsection_index_files.append( | ||
| os.path.join( | ||
| '/', gallery_dir, subsection, 'index.rst' |
There was a problem hiding this comment.
Should this have been os.path.sep rather than / (to make things work properly on Windows)? Or something else?
There was a problem hiding this comment.
You think you are right!
There was a problem hiding this comment.
Either that or '/'.join(..) would make sense. Do you have a Windows machine or VM where you could test and try to replicate the failure? If not, I can do it
There was a problem hiding this comment.
Sorry I don't have a Windows machine!
I had a quick look at the rest of the PR to see if other os.path.sep were missing, but I guess this would pop out on the build you are currently trying to run.
|
Yaay, I am very happy this was merged! I expect some backfire after this is released, but it's hard to assess how many projects will want to stick to a flat structure instead of a nested one. |
|
This change should be documented in the changelog in a didactic way (I must admit that with a quick look, I myself do not fully understand what it does).
|
|
Ha, I understand: this only affect the sidebar, right? If so, it's not a major breaking change, and my comment above does not hold. |
|
Yes, that's my understanding! @alexisthual do you want to try crafting a changelog entry for this? I agree it would be useful |
That is the goal 🙂 In short, it modifies generated toctrees in a way that mimics the real structure of sphinx galleries.
Eventually, it only adds some new Maybe some projects built their own custom sidebar by tweaking the generated toctree (precisely to display subsections), which I think is the only case that would require some refactoring. However, big documentations will benefit from this PR, as it will enable them to organise their sidebar items in categories (which is greatly needed for |
Yes, should I open a new PR to commit the changelog @larsoner ? |
|
Yep, that would be great |
|
If so, it's not a major breaking change, and my comment above does not hold.
Eventually, it only adds some new .rst files and intermediate toctrees, which I think are fair, non-breaking changes.
I agree it's fair.
Maybe some projects built their own custom sidebar by tweaking the generated toctree (precisely to display subsections),
We need to give a proper warning in the changelog.
However, big documentations will benefit from this PR,
No, I agree. It's very cool. We will use it in scikit-learn. I'm looking forward to seeing this released.
|
|
🎉 I'm super excited this is in! |
Haha, super happy to hear that, that's a relief 😅 |
|
This is a great achievement! 🎉 |
… to version 0.11.0 v0.11.0 ------- In this version, the "Out:" prefix applied to code outputs is now created from CSS pseudo-elements instead of additional real text. For more details, see `#896 <https://github.com/sphinx-gallery/sphinx-gallery/pull/896>`__. **Implemented enhancements:** Nesting gallery sections (i.e. gallery subfolders) was implemented in`#904 <https://github.com/sphinx-gallery/sphinx-gallery/pull/904>`__. This feature can be disabled (see config option ``nested_sections`` in the documentation) if the previous behaviour is prefered (`alexisthual <https://github.com/alexisthual>`__) Tooltips now overlay gallery items `commit 36166cd <https://github.com/sphinx-gallery/sphinx-gallery/pull/944/commits/36166cd2fc2b43ecbd585654cfe8745f3a1b3f64>`__. Custom CSS might need to be adapted (`alexisthual <https://github.com/alexisthual>`__). - Problem in section and example title level in subgalleries `#935 <https://github.com/sphinx-gallery/sphinx-gallery/issues/935>`__ - Add ability to write nested ``index.rst`` `#855 <https://github.com/sphinx-gallery/sphinx-gallery/issues/855>`__ (NEWS truncated at 15 lines)







Fixes #855.
Let's assume the gallery structure is the following (note the new
readme.rstfiles in each subsection):This PR:
index.rstfile for each subsection; it containsreadme.rst, the gallery items, and a hidden toctree of the gallery itemsOn-going tasks: