DOC: Enable nitpicky #14768

alexrudy · 2019-07-13T22:55:00Z

PR Summary

Enable Sphinx nitpicky mode, which raises a warning when a reference can't be found (and when combined with the -W option, causes an error when a reference can't be found).

There are currently ~2500 references which can't be resolved. I can't fix all of them in this SciPy sprint 🙁 but I did write a sphinx extension, missing_references which:

(a) records all of the missing references from one sphinx run in a JSON file.
(b) ignores those missing references on future runs.

This should mean that we can turn Sphinx nitpicky on for now, and it will emit a warning for new problems.

PR Checklist

Has Pytest style unit tests
Code is Flake 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

doc/sphinxext/missing_references.py

alexrudy · 2019-07-16T01:02:07Z

@anntzer I've added a location string to the JSON file – its not perfect (it replaces paths not within matplotlib with <external> and some paths end up as <unknown>).

Unfortunately, nitpicky can't use the location in the docs when suppressing warnings, so it doesn't impact the warning suppression (it will suppress all instances of the broken reference) but the filenames and line numbers should help when fixing these references in the future.

anntzer · 2019-07-16T07:19:38Z

Would it make sense to publish the sphinx extension separately? (I don't really mind either way)
I'm not sure it makes sense to commit the actual json file to the repo, unless we have a way to make sure it doesn't go out of date (forcing contributors to do a full doc build for docstrings edits just to keep the file up to date may raise the bar for contribution a bit :()
There's a bunch of entries that I don't actually understand, e.g. towards the top:

  [
    "<unknown>:1",
    "py:class",
    "_FancyAxislineStyle.FilledArrow"
  ],
  [
    "<unknown>:1",
    "py:class",
    "_FancyAxislineStyle.SimpleArrow"
  ],

FilledArrow's docstring is

The artist class that will be returned for SimpleArrow style.

so there's no markup problems there?

alexrudy · 2019-07-16T14:33:16Z

This could make sense as separate sphinx extension, but I'm not yet totally convinced of its wide-ranging applicability. It really only makes a difference for projects which haven't enabled nitpicky mode yet.
I think the point of this PR is the JSON file – it encodes the list of currently broken references. By including it, and not regenerating it again, we enforce that new references must be correct (they are subject to the nitpicky setting) but previously broken references can be fixed progressively over time. We only need to regenerate the JSON file when we fix a previously broken reference. And even then, if a contributor doesn't do so, there isn't much harm done (the JSON file specifies missing references to ignore, but now-correct references will be checked. I suppose a future PR could re-introduce a broken reference in the interim, but I would propose we live with that risk for now).
These entries aren't about mis-formatted docstrings, but about incorrect references to those objects. I'm not sure what drives the <unknown> (lots of things can inject RST into the sphinx parser and not set the filename sensibly) but it suggests that somewhere there is a reference of the form :py:class:`_FancyAxislineStyle.FilledArrow` but that reference isn't being resolved correctly. Usually, this is because the reference was underspecified (maybe it should have been ~mpl_toolkits.axisartist.axisline_style._FancyAxislineStyle.FilledArrow for the reference to resolve correctly, I'm not sure)

anntzer · 2019-07-16T14:46:43Z

This could make sense as separate sphinx extension, but I'm not yet totally convinced of its wide-ranging applicability. It really only makes a difference for projects which haven't enabled nitpicky mode yet.

Either way is fine, it's not very big and we'd not be exposing it as public API anyways.

I think the point of this PR is the JSON file – it encodes the list of currently broken references. By including it, and not regenerating it again, we enforce that new references must be correct (they are subject to the nitpicky setting) but previously broken references can be fixed progressively over time. We only need to regenerate the JSON file when we fix a previously broken reference. And even then, if a contributor doesn't do so, there isn't much harm done (the JSON file specifies missing references to ignore, but now-correct references will be checked. I suppose a future PR could re-introduce a broken reference in the interim, but I would propose we live with that risk for now).

Ah, I missed that non-listed entries would be subjected to nitpicking, that's good.
Perhaps you could fail the build if there are entries in the json file that are not being used, and list in the output? Then the circleci build would have something like

The following entries in missing-references.json are not used and should be deleted:
...

and then one can just delete them manually from the json file.

These entries aren't about mis-formatted docstrings, but about incorrect references to those objects. I'm not sure what drives the (lots of things can inject RST into the sphinx parser and not set the filename sensibly) but it suggests that somewhere there is a reference of the form :py:class:_FancyAxislineStyle.FilledArrow but that reference isn't being resolved correctly. Usually, this is because the reference was underspecified (maybe it should have been ~mpl_toolkits.axisartist.axisline_style._FancyAxislineStyle.FilledArrow for the reference to resolve correctly, I'm not sure)

Grepping for FilledArrow in the codebase yields

$ git grep -C5 FilledArrow
lib/mpl_toolkits/axisartist/axisline_style.py-            extended_path = self._extend_path(path_in_disp,
lib/mpl_toolkits/axisartist/axisline_style.py-                                              mutation_size=mutation_size)
lib/mpl_toolkits/axisartist/axisline_style.py-            self._path_original = extended_path
lib/mpl_toolkits/axisartist/axisline_style.py-            FancyArrowPatch.draw(self, renderer)
lib/mpl_toolkits/axisartist/axisline_style.py-
lib/mpl_toolkits/axisartist/axisline_style.py:    class FilledArrow(SimpleArrow):
lib/mpl_toolkits/axisartist/axisline_style.py-        """
lib/mpl_toolkits/axisartist/axisline_style.py-        The artist class that will be returned for SimpleArrow style.
lib/mpl_toolkits/axisartist/axisline_style.py-        """
lib/mpl_toolkits/axisartist/axisline_style.py-        _ARROW_STYLE = "-|>"
lib/mpl_toolkits/axisartist/axisline_style.py-
--
lib/mpl_toolkits/axisartist/axisline_style.py-                                           line_mutation_scale=self.size)
lib/mpl_toolkits/axisartist/axisline_style.py-            return axisline
lib/mpl_toolkits/axisartist/axisline_style.py-
lib/mpl_toolkits/axisartist/axisline_style.py-    _style_list["->"] = SimpleArrow
lib/mpl_toolkits/axisartist/axisline_style.py-
lib/mpl_toolkits/axisartist/axisline_style.py:    class FilledArrow(SimpleArrow):
lib/mpl_toolkits/axisartist/axisline_style.py:        ArrowAxisClass = _FancyAxislineStyle.FilledArrow
lib/mpl_toolkits/axisartist/axisline_style.py-
lib/mpl_toolkits/axisartist/axisline_style.py:    _style_list["-|>"] = FilledArrow

so it's not clear what could be done to fix that entry, for example.

Just to be clear, I think this would be a nice feature, I'm just trying to make sure we can use it efficiently :)

jklymak · 2019-07-24T13:43:34Z

As someone who is prone to having malformed references in the docs I'd be happy t see this merged. Is it ready for review?

alexrudy · 2019-07-24T18:49:25Z

I am working on two last small pieces –

Recording files & line numbers for warnings
Emitting a warning for unused warning suppressions in the .json doc.

timhoffm · 2019-07-24T20:20:25Z

@alexrudy Thanks for working on this. I've marked this as "work in progress" so that reviewers don't spend extra time on this until it's ready. Please report back when you think this is ready for review.

alexrudy · 2019-07-25T16:58:09Z

I think I'm ready for some more eyes on this!

Where we stand: @anntzer had a good request – record where warnings came from. That required a bit more work inside the extension, but now we get two things:

A better idea of where the missing references can be found – look through the missing_references.json to understand.
A warning when a reference is no longer missing – meaning that if you fix a reference in the docs, you will also need to remove that reference from the .json file.

@timhoffm – I can't add or remove labels, but removing the WIP label seems appropriate now.

anntzer · 2019-07-25T17:12:48Z

This is definitely much better now :) looking at missing-references.json under e.g. the py:obj key, one can easily find fixable references. A few more requests though:

looks like intersphinx is not handled properly? for example there is

    "numpy.hamming": [
      "lib/matplotlib/axes/_axes.py:docstring of matplotlib.axes.Axes.angle_spectrum:19",

but https://matplotlib.org/devdocs/api/_as_gen/matplotlib.axes.Axes.angle_spectrum.html actually links correctly to numpy.angle_spectrum. Likewise for some stdlib references.

looks like a missing std:viewcode entry is recorded for each and every element, but this is obviously incorrect as the [source] links are working fine (again, e.g. from https://matplotlib.org/devdocs/api/_as_gen/matplotlib.axes.Axes.angle_spectrum.html); for this specific point I guess you could even just manually exclude them...
there's a bunch of missing references from autosummary e.g.

    "matplotlib.animation.AVConvFileWriter.bin_path": [
      "doc/api/_as_gen/matplotlib.animation.AVConvFileWriter.rst:28:<autosummary>:1"

which likely(?) come from the fact that in https://matplotlib.org/devdocs/api/_as_gen/matplotlib.animation.AVConvFileWriter.html, under the Methods section, autosummary tries to link to AVConvFileWriter but that should go to the parent class' doc, https://matplotlib.org/devdocs/api/_as_gen/matplotlib.animation.MovieWriter.html#matplotlib.animation.MovieWriter.bin_path; not sure whether this should be filed as a sphinx bug?

perhaps sort the results alphabetically?

alexrudy · 2019-07-25T17:32:45Z

Hmm – @anntzer I think you have identified something more fundamental. I wonder if some of these references are being resolved by the sphinx missing-references hook via other extensions. I will have to investigate whether sphinx supports the ordering of extension hook calls.

alexrudy · 2019-07-25T22:25:04Z

Alright, one more attempt at this!

Since last time:

Record missing references primarily using a logging filter (not the missing-references sphinx event). This is because sphinx calls all event callbacks, even if it only needs the first successful result, so other extensions which used the missing-references callback (e.g. viewcode) to resolve references would report missing references to this extension.
Still use the event to ensure that reference is no longer missing – if any ignored reference doesn't trigger missing-references, then it doesn't need to be ignored. We can't use the logging filter in this case, because ignored references are never sent to the logger!

Two major outcomes from this:

The list of ignored references is much easier to understand and fix over time.
The missing references monitoring now depends not only on sphinx events but also on sphinx's logging setup. I think that is okay because (a) this is an internal sphinx extension and (b) sphinx logging has been pretty stable for a while and no class we access is private (though they aren't part of the extension hooks).

If I/we ever want to break this extension out into its own repo, I would want to ensure that upstream sphinx adds better support for this kind of thing.

anntzer · 2019-07-25T22:38:06Z

doc/sphinxext/missing_references.py

+        path = os.path.relpath(path, start=basepath)
+
+        if path.startswith(os.path.pardir):
+            path = os.path.join("<external>", os.path.basename(path))


I think this will give a different result if someone builds on Windows, which we actually don't want? So force the use of "/" here? (I would use pathlib.Path.as_posix because pathlib is nice :), but you don't have to.)

Oh, good catch! I'll switch to pathlib, I like that too.

I wanted to go with pathlib, but I decided against it, because the semantics of Path.relative_to aren't the same as os.path.relpath (the former raises an error for paths which can't be constructed as children of the base directory) – so it was easier just to pull out posixpath.

if you are relying on relpath having different semantics from Path.relative_to (haven't looked in depth) you should leave a comment so that the code doesn't "helpfully" get rewritten to use relative_to in the future...

otoh, you know you can pass pathlib Paths to os.path.relpath? alternatively, something like if basepath in path.parents: ...

I guess if you do use pathlib then PurePosixPath is the way to go to have forward slashes all the time, as you're not going to do any filesystem ops on them.

anntzer · 2019-07-25T22:43:12Z

Nice! Just one review point left I think, I'm going to skip style nitpicks unless you want to hear them :)

alexrudy · 2019-07-25T23:08:03Z

Always happy to hear any nitpicks, @anntzer !

anntzer

none of these style comments should be considered too seriously :)

doc/sphinxext/missing_references.py

anntzer · 2019-07-26T06:49:30Z

doc/sphinxext/missing_references.py

+
+    if path:
+
+        basepath = os.path.abspath(os.path.join(app.confdir, ".."))


actually I think this will fail if building the docs against a non-editable install? (so that you'd find the wrong matplotlib by going just up once from doc/conf.py) Admittedly a bit theoretical [though note that conda or linux packagers have been known to do stranger stuff], but you could possibly instead normalize the paths relative to Path(matplotlib.__init__.__file__).parent.parent (which helpfully also works for mpl-toolkits) instead (I guess if this was to be a mpl-independent extension you'd need to make this a config option, but note that in such a case your current approach also fails if conf.py is under doc/source/conf.py rather than doc/conf.py, which is a valid sphinx layout).

Ahh – yes, this makes sense. A very good catch.

I'm going to test your suggestion and see what happens. It definitely won't work if this becomes a true sphinx extension, but there are are a few ways around that in the future.

I've tried some things out here –

It's quite difficult to gracefully handle building the docs correctly when matplotlib isn't an editable install. Figuring out what is a "matplotlib" path vs any other module, and producing paths which match the editable install case, is very difficult. e.g. in the source tree mode, we have a path lib/matplotlib/axes/_axes.py, but in installed mode, this is something like site-packages/matplotlib/axes/_axes.py. Although we could normalize those two paths, it is a lot of work for a particular edge case, for functionality that isn't very important to this extension. (The features of this extension are already ballooning beyond what I had envisioned).

Instead, I propose that we detect when the matplotlib source directory doesn't seem to be in the right place relative to the docs directory and in those cases, we don't worry about warning for unused ignores. Everything else should still work fine in those cases – the actual process of ignoring unmatched references doesn't use the file paths at all.

Sorry to have sent you on a wild goose chase... just ignoring warnings if not in a standard layout is perfectly fine.

No worries – it was interesting idea to try. I'm just trying to balance making things useful with making the perfect sphinx extension!

QuLogic

Some typos and other clarifications.

QuLogic · 2019-07-26T08:22:24Z

doc/sphinxext/missing_references.py

+        self.app = app
+        super().__init__()
+
+    def _record_reference(self, record):


Not sure why you need this separate method when it only has one caller, which is also a method on the same class.

Its done as a convenience – the filter() method must return True or False, but the _record_reference method doesn't actually do any log filtering (all messages should get logged) so its a bit clearer IMO to return True from the filter method and put the logic somewhere else (so that we don't have to ensure that early returns from _record_reference also return True).

doc/sphinxext/missing_references.py

QuLogic · 2019-07-26T08:36:23Z

doc/sphinxext/missing_references.py

+    missing_reference_filter = MissingReferenceFilter(app)
+    for handler in sphinx_logger.handlers[:]:
+        if (isinstance(handler, sphinx_logging.WarningStreamHandler) and
+            missing_reference_filter not in handler.filters):


Word-wrapped if should be indented one more level.

doc/sphinxext/missing_references.py

anntzer · 2019-07-28T08:05:55Z

doc/sphinxext/missing_references.py

+        for ignored_reference_location in locations:
+            if (ignored_reference_location not in
+                    missing_reference_locations):
+                msg = (f"Reference {domain_type} {target} for "


indent a bit weird

Agreed – @QuLogic what were you going for with your earlier comment about this if statement?

This indent is correct now; I don't know what @anntzer finds weird.

Though I suppose I could point out that the if condition could fit on one line.

It's the f-string indent which is currently

msg = (f"Reference {domain_type} {target} for " f"{ignored_reference_location} can be removed" f" from {app.config.missing_references_filename}." "It is no longer a missing reference in the docs.")

but should rather be something like

msg = (f"Reference {domain_type} {target} for " f"{ignored_reference_location} can be removed" f" from {app.config.missing_references_filename}." "It is no longer a missing reference in the docs.")

doc/sphinxext/missing_references.py

anntzer

enough nitpicking :p

alexrudy · 2019-07-29T16:14:35Z

I'm going to rebase and fix the failing tests on this.

timhoffm

Ok, let's give this a try. 😄

alexrudy force-pushed the enable-nitpicky branch 2 times, most recently from a61c555 to 528c47f Compare July 14, 2019 16:39

jklymak reviewed Jul 15, 2019

View reviewed changes

doc/sphinxext/missing_references.py Show resolved Hide resolved

anntzer reviewed Jul 15, 2019

View reviewed changes

doc/sphinxext/missing_references.py Outdated Show resolved Hide resolved

anntzer added the Documentation label Jul 16, 2019

anntzer mentioned this pull request Jul 16, 2019

Fix some broken refs in the docs. #14802

Merged

6 tasks

timhoffm added the status: work in progress label Jul 24, 2019

anntzer removed the status: work in progress label Jul 25, 2019

anntzer reviewed Jul 25, 2019

View reviewed changes

anntzer reviewed Jul 26, 2019

View reviewed changes

doc/sphinxext/missing_references.py Outdated Show resolved Hide resolved

anntzer reviewed Jul 26, 2019

View reviewed changes

QuLogic reviewed Jul 26, 2019

View reviewed changes

alexrudy force-pushed the enable-nitpicky branch from 184d938 to 74af5b6 Compare July 28, 2019 05:44

anntzer reviewed Jul 28, 2019

View reviewed changes

doc/sphinxext/missing_references.py Outdated Show resolved Hide resolved

anntzer reviewed Jul 28, 2019

View reviewed changes

doc/sphinxext/missing_references.py Outdated Show resolved Hide resolved

anntzer approved these changes Jul 28, 2019

View reviewed changes

alexrudy force-pushed the enable-nitpicky branch from 66db761 to 2e270bd Compare July 29, 2019 16:38

alexrudy added 3 commits July 30, 2019 23:13

Add extension to record missing references

1d330e0

Enable nitpicky mode

9e0ffe1

Add location to missing references

6d18a51

alexrudy force-pushed the enable-nitpicky branch from 2e270bd to 0c725e2 Compare July 31, 2019 06:13

Record references via logging

6fab2e0

alexrudy force-pushed the enable-nitpicky branch from 0c725e2 to 6fab2e0 Compare July 31, 2019 17:07

Improve reference checkign

8f37c92

anntzer mentioned this pull request Aug 8, 2019

Don't generate individual doc entries for inherited Axes/Axis/Tick methods #14960

Merged

6 tasks

timhoffm approved these changes Aug 8, 2019

View reviewed changes

timhoffm merged commit 4c8aadc into matplotlib:master Aug 8, 2019

timhoffm mentioned this pull request Aug 18, 2019

sphinxext.missing_references does not specify if it supports parallel file read. #15075

Closed

QuLogic added this to the v3.2.0 milestone Aug 25, 2019


		if path:

		basepath = os.path.abspath(os.path.join(app.confdir, ".."))

DOC: Enable nitpicky #14768

DOC: Enable nitpicky #14768

Conversation

alexrudy commented Jul 13, 2019

PR Summary

PR Checklist

alexrudy commented Jul 16, 2019

anntzer commented Jul 16, 2019

alexrudy commented Jul 16, 2019 • edited Loading

anntzer commented Jul 16, 2019 • edited Loading

jklymak commented Jul 24, 2019

alexrudy commented Jul 24, 2019

timhoffm commented Jul 24, 2019

alexrudy commented Jul 25, 2019

anntzer commented Jul 25, 2019 • edited Loading

alexrudy commented Jul 25, 2019

alexrudy commented Jul 25, 2019

anntzer Jul 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer commented Jul 25, 2019

alexrudy commented Jul 25, 2019

anntzer left a comment

Choose a reason for hiding this comment

anntzer Jul 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuLogic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer left a comment

Choose a reason for hiding this comment

alexrudy commented Jul 29, 2019

timhoffm left a comment

Choose a reason for hiding this comment

alexrudy commented Jul 16, 2019 •

edited

Loading

anntzer commented Jul 16, 2019 •

edited

Loading

anntzer commented Jul 25, 2019 •

edited

Loading

anntzer Jul 25, 2019 •

edited

Loading

anntzer Jul 26, 2019 •

edited

Loading