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: Use official numpydoc extension #24098

Merged
merged 11 commits into from
Dec 15, 2018

Conversation

FHaase
Copy link
Contributor

@FHaase FHaase commented Dec 4, 2018

  • Replace custom numpydoc in doc/sphinxext/numpydoc with official
    numpydoc release
  • Remove numpydoc_use_blockquotes parameter

 - Replace custom numpydoc in `doc/sphinxext/numpydoc` with official
   numpydoc release
 - Remove `numpydoc_use_blockquotes` parameter

Signed-off-by: Fabian Haase <haase.fabian@gmail.com>
Signed-off-by: Fabian Haase <haase.fabian@gmail.com>
@pep8speaks
Copy link

pep8speaks commented Dec 4, 2018

Hello @FHaase! Thanks for updating the PR.

Comment last updated on December 14, 2018 at 18:25 Hours UTC

@FHaase
Copy link
Contributor Author

FHaase commented Dec 4, 2018

@datapythonista is this what you meant in #23763 with:

avoid that numpydoc parameters are rendered inside a <blockquote> tag, breaking the formatting

Before:
numpydoc-before

After:
numpydoc-after

@FHaase FHaase changed the title DOC: Use official numpydoc package DOC: Use official numpydoc extension Dec 4, 2018
@TomAugspurger
Copy link
Contributor

Some xref issues for context #11257, #6003, numpy/numpydoc#106 (comment).

@FHaase can you verify that those docstrings (e.g. Index.str) are rendering correctly?

Signed-off-by: Fabian Haase <haase.fabian@gmail.com>
@codecov
Copy link

codecov bot commented Dec 4, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #24098   +/-   ##
=======================================
  Coverage   42.52%   42.52%           
=======================================
  Files         161      161           
  Lines       51697    51697           
=======================================
  Hits        21982    21982           
  Misses      29715    29715
Flag Coverage Δ
#single 42.52% <ø> (ø) ⬆️

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 1d3ed91...b39f68e. Read the comment docs.

@codecov
Copy link

codecov bot commented Dec 4, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #24098   +/-   ##
=======================================
  Coverage   92.22%   92.22%           
=======================================
  Files         162      162           
  Lines       51828    51828           
=======================================
  Hits        47798    47798           
  Misses       4030     4030
Flag Coverage Δ
#multiple 90.62% <ø> (ø) ⬆️
#single 43% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/ops.py 94.26% <ø> (ø) ⬆️

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 040f06f...afeb025. Read the comment docs.

@FHaase
Copy link
Contributor Author

FHaase commented Dec 4, 2018

Index.str renders identical with release version of 0.23.4

We had an option to use a member_list over param_list to display the Attribute section which made
the attribute section look like the Methods section rather than the Parameter section.

The option is removed in favor of having this behavior always. So pandas.Index renders unchanged.

'notes': self._str_section('Notes'),
'references': self._str_references(),
'examples': self._str_examples(),
'attributes': self._str_member_list('Attributes'),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously a condition existed:

            'attributes':
                self._str_param_list('Attributes', fake_autosummary=True)
                if self.attributes_as_param_list
                else self._str_member_list('Attributes'),

I assumed that self.attributes_as_param_list was always set as False

@datapythonista
Copy link
Member

I don't understand this PR. Removing our copy of numpydoc and importing it as a dependency sounds good (not sure why was copied, I understand from the linked issues that it was because py3 compatibility?)

But what you're doing is have it as a dependency, and create an intermediate module to monkeypatch it? What is the advantage?

@datapythonista
Copy link
Member

Regarding the blockquotes, the html wraps lists with a <blockquote>, because a list is indented, and indented code is used for a quote too. With the current style is not visible (I think we changed the css or something), but with the new theme I'm implementing the lists are rendered with a different font (because of the blockquote). See here:

bloquote

Signed-off-by: Fabian Haase <haase.fabian@gmail.com>
@FHaase
Copy link
Contributor Author

FHaase commented Dec 5, 2018

But what you're doing is have it as a dependency, and create an intermediate module to monkeypatch it? What is the advantage?

Moved the patching to conf.py. Didn't know that replacing a function has effect on a global level.

@@ -420,6 +413,36 @@
]


# Modify numpydoc to show Attributes section like Methods section
# PANDAS HACK: Replace attributes param_list by member_list
Copy link
Member

Choose a reason for hiding this comment

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

This looks much better. But I don't think the comment is clear enough to understand what/why we are doing this. I'd probably move it to the docstring of alternative_str and expand it (and I'd probably rename alternative_str to sphinxdocstring_str).

Signed-off-by: Fabian Haase <haase.fabian@gmail.com>
@gfyoung gfyoung added the Docs label Dec 6, 2018
@FHaase
Copy link
Contributor Author

FHaase commented Dec 6, 2018

Regarding the blockquotes, the html wraps lists with a <blockquote>, because a list is indented, and indented code is used for a quote too. With the current style is not visible (I think we changed the css or something), but with the new theme I'm implementing the lists are rendered with a different font (because of the blockquote).

@datapythonista I've compared the text in io.rst starting at line 1880 with the docutils example. When I indent by 2 spaces instead of 4 the <blockquote> doesn't get generated anymore. Can you confirm this?

If the JSON serializer cannot handle the container contents directly it will
fall back in the following manner:

* if the dtype is unsupported (e.g. ``np.complex``) then the ``default_handler``, if provided, will be called
  for each value, otherwise an exception is raised.

* if an object is unsupported it will attempt the following:


  * check if the object has defined a ``toDict`` method and call it.
    A ``toDict`` method should return a ``dict`` which will then be JSON serialized.

  * invoke the ``default_handler`` if one was provided.

  * convert the object to a ``dict`` by traversing its contents. However this will often fail
    with an ``OverflowError`` or give unexpected results.

@datapythonista
Copy link
Member

I think the reStructuredText standard requires no indentation for first level lists, and 4 for sublists. We had some problems with the rendering in the past, you can take a look at #21520 and referenced issues, I don't remember the details.

@FHaase
Copy link
Contributor Author

FHaase commented Dec 6, 2018

with-blockquote

<ul>
<li><p class="first">if the dtype is unsupported (e.g. <code class="docutils literal notranslate"><span class="pre">np.complex</span></code>) then the <code class="docutils literal notranslate"><span class="pre">default_handler</span></code>, if provided, will be called
for each value, otherwise an exception is raised.</p>
</li>
<li><p class="first">if an object is unsupported it will attempt the following:</p>
<blockquote>
<div><ul class="simple">
<li>check if the object has defined a <code class="docutils literal notranslate"><span class="pre">toDict</span></code> method and call it.
A <code class="docutils literal notranslate"><span class="pre">toDict</span></code> method should return a <code class="docutils literal notranslate"><span class="pre">dict</span></code> which will then be JSON serialized.</li>
<li>invoke the <code class="docutils literal notranslate"><span class="pre">default_handler</span></code> if one was provided.</li>
<li>convert the object to a <code class="docutils literal notranslate"><span class="pre">dict</span></code> by traversing its contents. However this will often fail
with an <code class="docutils literal notranslate"><span class="pre">OverflowError</span></code> or give unexpected results.</li>
</ul>
</div></blockquote>
</li>
</ul>

vs

without-blockquote

<ul class="simple">
<li>if the dtype is unsupported (e.g. <code class="docutils literal notranslate"><span class="pre">np.complex</span></code>) then the <code class="docutils literal notranslate"><span class="pre">default_handler</span></code>, if provided, will be called
for each value, otherwise an exception is raised.</li>
<li>if an object is unsupported it will attempt the following:<ul>
<li>check if the object has defined a <code class="docutils literal notranslate"><span class="pre">toDict</span></code> method and call it.
A <code class="docutils literal notranslate"><span class="pre">toDict</span></code> method should return a <code class="docutils literal notranslate"><span class="pre">dict</span></code> which will then be JSON serialized.</li>
<li>invoke the <code class="docutils literal notranslate"><span class="pre">default_handler</span></code> if one was provided.</li>
<li>convert the object to a <code class="docutils literal notranslate"><span class="pre">dict</span></code> by traversing its contents. However this will often fail
with an <code class="docutils literal notranslate"><span class="pre">OverflowError</span></code> or give unexpected results.</li>
</ul>
</li>
</ul>

@FHaase
Copy link
Contributor Author

FHaase commented Dec 6, 2018

From A ReStructuredText Primer at bulleted list

* a bullet point using "*"

  - a sub-list using "-"

    + yet another sub-list

  - another item

From Sphinx Documentation

* this is
* a list

  * with a nested list
  * and some subitems

* and here the parent list continues

Both use 2 spaces as indent.

@datapythonista
Copy link
Member

See #21518 (the problem I fixed at that time was that some top level lists were indented). Not sure where I checked the standard, but for what I saw, while sphinx is flexible, the standard is 4 spaces, and other software may fail to render the lists with a different number.

Signed-off-by: Fabian Haase <haase.fabian@gmail.com>
@FHaase
Copy link
Contributor Author

FHaase commented Dec 6, 2018

You mentioned pandoc in that issue, looking at the documentation pandoc uses 2 spaces as well.

* fruits
  + apples
    - macintosh
    - red delicious
  + pears
  + peaches
* vegetables
  + broccoli
  + chard

I don't know from where you took 4 spaces as indentation for sublists, but docutils, sphinx, pandoc, ...
all seem to use 2.

pandoc uses 4 spaces for verbatim text:

A block of text indented four spaces (or one tab) is treated as verbatim text: that is, special characters do not trigger special formatting, and all spaces and line breaks are preserved. For example, ...

Copy link
Member

@datapythonista datapythonista 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 @FHaase

Regarding the lists indentation, I guess I was wrong, not sure where I got the information from. But let's better move the discussion to the appropriate issue, so we have it there when someone works in it.

@datapythonista datapythonista self-assigned this Dec 7, 2018
@TomAugspurger
Copy link
Contributor

cc @jorisvandenbossche.

I haven't had a chance to look at this yet. Will try to next week.

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

yeah @jorisvandenbossche needs to have a look at this. IIRC there were some rationale for using the vendored version.

@@ -21,6 +21,7 @@ dependencies:
- notebook
- numexpr
- numpy=1.13*
- numpydoc
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have a min version required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know exactly. Currently it uses the latest which is 0.8.0.

@jorisvandenbossche
Copy link
Member

I am certainly fine with monkeypatching upstream numpydoc, instead of vendoring the full package as we do now. That makes it a bit more explicit which hacks we added / where the changes are compared to upstream.

I think in the past we had more hacks than just the attributes listing, but it might be that the others already have been solved in the meantime.
I quickly looked at the build log, and we have a whole bunch of warnings related to numpydoc / autodoc, but we also have them on master, so this PR does not seem to add new warnings.

@jorisvandenbossche
Copy link
Member

Reading through #20383, it seems I have updated our vendored version to the upstream one earlier this year, with the attribute listing as the only 'hack' afterwards. And in that way removing the patches we had previously (#11257)

There is a note there (#20383 (comment)) about the numpydoc_use_blockquotes and **kwargs in the docstring. @FHaase this is still working now? (as I see you removed that setting; I don't really remember directly why it was needed at the time)

We had an option to use a member_list over param_list to display the Attribute section which made
the attribute section look like the Methods section rather than the Parameter section.

The option is removed in favor of having this behavior always.

@FHaase yes, fine to remove that. I originally did it like that in the hope to push that change upstream (for which there is a PR: numpy/numpydoc#161)

@jreback
Copy link
Contributor

jreback commented Dec 7, 2018

@jorisvandenbossche over to you; merge when satisfied

@FHaase
Copy link
Contributor Author

FHaase commented Dec 7, 2018

There are two pictures added above. With a before and after comparison.
They feature a *args and **kwargs in parameters section. Which does look fine to me.

The vendored version was 0.8.0.dev0 (something like that) now it's 0.8.0. I guess they fixed all those issues for the release version.

Signed-off-by: Fabian Haase <haase.fabian@gmail.com>
@FHaase
Copy link
Contributor Author

FHaase commented Dec 11, 2018

Patched numpy/numpydoc#150 into SphinxDocString.
Running python make.py html --single api doesn't generate any WARNING: Inline strong start-string without end-string.

Happened those issues were cut off after all the Warnings regarding the toctree were shown.

@jorisvandenbossche
Copy link
Member

Signed-off-by: Fabian Haase <haase.fabian@gmail.com>
@jreback jreback added this to the 0.24.0 milestone Dec 13, 2018
@jreback
Copy link
Contributor

jreback commented Dec 13, 2018

lgtm. can you merge master and ping on green. any issues closed by this?

@FHaase
Copy link
Contributor Author

FHaase commented Dec 14, 2018

Ping @jreback

@jreback
Copy link
Contributor

jreback commented Dec 14, 2018

https://travis-ci.org/pandas-dev/pandas/jobs/468121566

this failed building docs, and didn't fail the jobs: @datapythonista

@datapythonista
Copy link
Member

I saw that couple of days ago working in some fixes to the doc build. We're using os.system() to call sphinx-build, and it returns the status code instead of raising an exception when the called command fails.

That will be fixed in the changes I'm doing, hopefully I'll be able to open a PR very soon, just need to fix some problems I'm getting with sphinx-autogen.

@jreback
Copy link
Contributor

jreback commented Dec 14, 2018

ok, but it appears our docs are completely broken now. so let's fix that before merging anything else.

@datapythonista
Copy link
Member

I thought the failure was in this PR. Let me take a look then.

@jreback
Copy link
Contributor

jreback commented Dec 14, 2018

@datapythonista
Copy link
Member

locally it fails because of the enchant (spellcheck) thing. Not sure if that's the only problem, as the error in the CI is different, but I'll get rid of that first, as it's something I wanted to do anyway (it can't be used, as the dependencies can't be installed)

@datapythonista
Copy link
Member

I've got a different error locally, but I think something is wrong in my system (I'm getting pytest segfaults in an unrelated branch).

I opened #24287 which with some luck fixed the problem. And in any case should let us know if the doc build fails.

@jreback jreback merged commit a7bc7eb into pandas-dev:master Dec 15, 2018
@jreback
Copy link
Contributor

jreback commented Dec 15, 2018

thanks @FHaase

@FHaase FHaase deleted the numpydoc-disable-blockquotes branch December 17, 2018 13:55
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
* Use official numpydoc package

 - Replace custom numpydoc in `doc/sphinxext/numpydoc` with official
   numpydoc release
 - Remove `numpydoc_use_blockquotes` parameter

Signed-off-by: Fabian Haase <haase.fabian@gmail.com>

* Add numpydoc package to travis

Signed-off-by: Fabian Haase <haase.fabian@gmail.com>

* Use member listing for attributes

Signed-off-by: Fabian Haase <haase.fabian@gmail.com>

* Move modification of SphinxDocString to conf.py

Signed-off-by: Fabian Haase <haase.fabian@gmail.com>

* Update comments to be more descriptive

Signed-off-by: Fabian Haase <haase.fabian@gmail.com>

* Fix linting

Signed-off-by: Fabian Haase <haase.fabian@gmail.com>

* Review of @jorisvandenbossche

Signed-off-by: Fabian Haase <haase.fabian@gmail.com>

* Fix linting

Signed-off-by: Fabian Haase <haase.fabian@gmail.com>
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
* Use official numpydoc package

 - Replace custom numpydoc in `doc/sphinxext/numpydoc` with official
   numpydoc release
 - Remove `numpydoc_use_blockquotes` parameter

Signed-off-by: Fabian Haase <haase.fabian@gmail.com>

* Add numpydoc package to travis

Signed-off-by: Fabian Haase <haase.fabian@gmail.com>

* Use member listing for attributes

Signed-off-by: Fabian Haase <haase.fabian@gmail.com>

* Move modification of SphinxDocString to conf.py

Signed-off-by: Fabian Haase <haase.fabian@gmail.com>

* Update comments to be more descriptive

Signed-off-by: Fabian Haase <haase.fabian@gmail.com>

* Fix linting

Signed-off-by: Fabian Haase <haase.fabian@gmail.com>

* Review of @jorisvandenbossche

Signed-off-by: Fabian Haase <haase.fabian@gmail.com>

* Fix linting

Signed-off-by: Fabian Haase <haase.fabian@gmail.com>
@larsoner larsoner mentioned this pull request Apr 2, 2019
5 tasks
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.

7 participants