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

DOC: restore toctree maxdepth #25134

Merged
merged 7 commits into from
Mar 11, 2019

Conversation

jorisvandenbossche
Copy link
Member

Closes #25131

I used a maxdepth of 3 now, instead of the original 4.

Now, the problem is that this makes the toctree on the main page (the one in the actual page body, not the one in the left sidebar) again very long and not very usable ..

cc @datapythonista @neili02

@jorisvandenbossche jorisvandenbossche added this to the 0.24.2 milestone Feb 4, 2019
@codecov
Copy link

codecov bot commented Feb 4, 2019

Codecov Report

Merging #25134 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #25134      +/-   ##
==========================================
+ Coverage   92.37%   92.37%   +<.01%     
==========================================
  Files         166      166              
  Lines       52408    52408              
==========================================
+ Hits        48411    48412       +1     
+ Misses       3997     3996       -1
Flag Coverage Δ
#multiple 90.79% <ø> (ø) ⬆️
#single 42.86% <ø> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/util/testing.py 88.13% <0%> (+0.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3c9d6e...ade2608. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 4, 2019

Codecov Report

Merging #25134 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #25134   +/-   ##
=======================================
  Coverage   91.26%   91.26%           
=======================================
  Files         173      173           
  Lines       52968    52968           
=======================================
  Hits        48339    48339           
  Misses       4629     4629
Flag Coverage Δ
#multiple 89.83% <ø> (ø) ⬆️
#single 41.71% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/config.py 87% <0%> (-0.05%) ⬇️
pandas/plotting/_core.py 83.53% <0%> (ø) ⬆️
pandas/core/arrays/sparse.py 92.17% <0%> (ø) ⬆️
pandas/core/groupby/base.py 91.83% <0%> (ø) ⬆️
pandas/core/base.py 97.76% <0%> (ø) ⬆️
pandas/core/frame.py 96.79% <0%> (ø) ⬆️
pandas/core/generic.py 93.65% <0%> (ø) ⬆️
pandas/io/feather_format.py 89.74% <0%> (ø) ⬆️
pandas/core/groupby/groupby.py 97.2% <0%> (ø) ⬆️
pandas/core/indexes/base.py 96.57% <0%> (ø) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e28ae70...789e7c7. Read the comment docs.

@datapythonista
Copy link
Member

-1 on this.

Didn't realise that the home page depth was affecting the sidebar levels (I understand that's this is what it's being fixed).

In doc/source/conf.py, in the header context we could add the tocdepth, which will be injected in every page. That's what it's supposed to control the sidebars.

Or I guess that creating an invisible toctree with depth 3 or 4 after this one should have the same effect (I guess the depth is being propagated from page to page after this toctree is being rendered).

@jorisvandenbossche
Copy link
Member Author

Didn't realise that the home page depth was affecting the sidebar levels (I understand that's this is what it's being fixed).

Yes, that is what is being fixed.
And IMO, the current within page navigation is quite broken. We have a lot of huge pages, and there is now no way to go to a specific section apart from scrolling.

we could add the tocdepth. That's what it's supposed to control the sidebars.

I am not sure that is what it is supposed to control. At least, from trying both in the pandas docs, as in a dummy test docs, it does not do that .. It's not really clear to me what it then actually does control.
But the sidebar maxdepth seems to be solely determined by the main toctree entry on the main index.rst. All other combinations of tocdepths and maxdepths on the other pages / toctrees (that I tried, there are probably more ... :)) didn't influence the sidebar.

Or I guess that creating an invisible toctree with depth 3 or 4 after this one should have the same effect (I guess the depth is being propagated from page to page after this toctree is being rendered).

Yes, I tried that as well, but it cannot be combined with another visible toctree with less depth. Because it will simply put the two toctrees in the sidebar ...


It's a trade-off between a better side-bar navigation and a shorter (more useful) table of contents on the main index page. Personally, I find the side bar more important.

The only solution I see at the moment is to make the main toctree :hidden:, and create a shorter overview manually for the main index page (without using the toctree directive). Basically what eg xarray does: http://xarray.pydata.org/en/stable/ (their sidebar expands additional levels, but the listing on the body of the main page only has one level -> that one is constructed manually).

@jorisvandenbossche
Copy link
Member Author

BTW, I opened an issue about the :tocdepth: on sphinx: sphinx-doc/sphinx#6033

@jorisvandenbossche
Copy link
Member Author

@datapythonista I implemented what I describe above (last paragraph in #25134 (comment)):

  • have the main index.rst toctree with a :maxdepth: of 3 instead of 2, but :hidden: -> it is used for the sidebar, but the long (with 3 levels) toctree is not shown on the front page main content
  • manually insert a toctree-like list to mimic the toctree with a :maxdepth: of 2

I needed to edit out sphinx theme template to include hidden toctrees in the sidebar

@jorisvandenbossche
Copy link
Member Author

@jreback @TomAugspurger I would like to include this for 0.24.2.

And before you complain about the added complexity (separate manual toctree to keep up to date): as far as I know, there is no way with sphinx to have both the sidebar toc go up to a level of 3, and have that same toctree shown on the main page with a depth up to a level of 2. See the discussion above.
I would love to hear a better solution, but I already spent quite some time trying to figure this out, and currently my conclusion is that it is not possible with sphinx.

@WillAyd
Copy link
Member

WillAyd commented Mar 10, 2019

@jorisvandenbossche thoughts on #25614 before this? I find the default TOC navigation that comes with that on the left hand side rather nice.

@@ -19,7 +19,7 @@
{%- block sidebar1 %}
{%- block sidebartoc %}
<h3>{{ _('Table Of Contents') }}</h3>
{{ toctree() }}
{{ toctree(includehidden=True) }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use a few hidden toctrees in the API docs to prevent sphinx /numpydoc from throwing a warning (e.g. reference/index.rst has api/pandas.api.extensions.ExtensionDtype.na_value. Do you know if those show up in the sidebar now? Or is the include_hidden limited to just certain pages?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. And yes, they do ...
Checking now if by putting them in a comment instead of using :hidden: helps for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option may be to put them in a different toctree on a page that is an orphan? Not sure if Sphinx will complain about that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both seemed to work, used the commented toctrees now.

@@ -52,3 +53,66 @@ See the :ref:`overview` for more detail about what's in the library.
development/index
whatsnew/index
{% endif -%}


* :doc:`whatsnew/v0.24.1`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the link we currently have on the main page (I only omitted the subsections for this page)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I suppose you meant the "0.24.1". That should indeed be 0.25.0 for master (and needs to be changed to 0.24.2 on the 0.24.x branch), thanks for the catch.

@jorisvandenbossche
Copy link
Member Author

thoughts on #25614 before this? I find the default TOC navigation that comes with that on the left hand side rather nice.

@WillAyd I am personally slighly -1 on hurrying that, but will try to answer there soon. But also, both PRs should be quite independent (except of the one-line change I made in the theme layout here, which would not be needed with the rtd theme. But the main part of this PR (changing the maxdepth etc) is also needed for the rtd theme)

@jorisvandenbossche
Copy link
Member Author

(failure is the sparse one which is fixed on master)

Going to merge this soon, any more comments?

@TomAugspurger
Copy link
Contributor

I think this is good to merge now (or soon. Maybe leave a couple hours).

This solves the biggest complain with the 0.24.0 reorganization. But it shouldn't preclude further improvements in navigation and restructuring of our very large pages.

@jorisvandenbossche
Copy link
Member Author

This solves the biggest complain with the 0.24.0 reorganization. But it shouldn't preclude further improvements in navigation and restructuring of our very large pages.

Yes, for sure. This is only meant for patching up what we currently have to make it more usable again, not to be a final solution.

@jorisvandenbossche jorisvandenbossche merged commit dc7b466 into pandas-dev:master Mar 11, 2019
@jorisvandenbossche jorisvandenbossche deleted the doc-toctree branch March 11, 2019 16:12
meeseeksmachine pushed a commit to meeseeksmachine/pandas that referenced this pull request Mar 11, 2019
thoo added a commit to thoo/pandas that referenced this pull request Mar 11, 2019
* upstream/master: (110 commits)
  DOC: hardcode contributors for 0.24.x releases (pandas-dev#25662)
  DOC: restore toctree maxdepth (pandas-dev#25134)
  BUG: Redefine IndexOpsMixin.size, fix pandas-dev#25580. (pandas-dev#25584)
  BUG: to_csv line endings with compression (pandas-dev#25625)
  DOC: file obj for to_csv must be newline='' (pandas-dev#25624)
  Suppress incorrect warning in nargsort for timezone-aware DatetimeIndex (pandas-dev#25629)
  TST: fix incorrect sparse test (now failing on scipy master) (pandas-dev#25653)
  CLN: Removed debugging code (pandas-dev#25647)
  DOC: require Return section only if return is not None nor commentary (pandas-dev#25008)
  DOC:Remove hard-coded examples from _flex_doc_SERIES (pandas-dev#24589) (pandas-dev#25524)
  TST: xref pandas-dev#25630 (pandas-dev#25643)
  BUG: Fix pandas-dev#25481 by fixing the error message in TypeError (pandas-dev#25540)
  Fixturize tests/frame/test_mutate_columns.py (pandas-dev#25642)
  Fixturize tests/frame/test_join.py (pandas-dev#25639)
  Fixturize tests/frame/test_combine_concat.py (pandas-dev#25634)
  Fixturize tests/frame/test_asof.py (pandas-dev#25628)
  BUG: Fix user-facing AssertionError with to_html (pandas-dev#25608) (pandas-dev#25620)
  DOC: resolve all GL03 docstring validation errors (pandas-dev#25525)
  TST: failing wheel building on PY2 and old numpy (pandas-dev#25631)
  DOC: Remove makePanel from docs (pandas-dev#25609) (pandas-dev#25612)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants