Repository navigation
MRG: Add refs from docstring to backrefs - #347
Conversation
Codecov Report
@@ Coverage Diff @@
## master #347 +/- ##
==========================================
+ Coverage 93.13% 95.31% +2.17%
==========================================
Files 27 27
Lines 1953 1985 +32
==========================================
+ Hits 1819 1892 +73
+ Misses 134 93 -41
Continue to review full report at Codecov.
|
0c1f0d4 to
49d4f11
Compare
|
I think this solves a big problem for Matplotlib for which we've been using a hacky workaround. matplotlib/matplotlib#10974 |
|
I'm happy to rebase, IIRC the only remaining thing is reviews (@choldgraf ?) |
|
holy moly I didn't notice this one in February, but agree it would be super useful. I'll try to give it a review during one of my "I've been talking about code way too much I need to actually code a bit" breaks at SciPy :-) |
|
Rebased and added fixes for our test infrastructure:
Example of New docs of feature: Ready for review/merge from my end. |
Looks nice! Just curious, would it be easy to make |
|
Possibly but that's a separate issue (exists in master too I think) so I'd
rather deal with it separately
I'm surprised intersphinx doesn't just take care of it actually
|
lesteve
left a comment
lesteve
left a comment
There was a problem hiding this comment.
This looks good, a few comments/questions.
| The files we are including are from the ``backreferences_dir`` | ||
| configuration option setup for Sphinx-Gallery. | ||
|
|
||
| .. _code_tesiii: |
There was a problem hiding this comment.
This reference is not used anywhere is it?
There was a problem hiding this comment.
I don't see it added anywhere, @Titan-C what is this for?
There was a problem hiding this comment.
Since I added in doc/_templates/module.rst also the instruction to document classes I wanted to seed how this was affected on the line numbers. This reference was for personal use when reloading the HTML should have not been committed.
As a side issue. It appears that the readthedocs theme on a python3 build(at least mine) cannot align the line numbers with the code lines. Other sphinx themes and our py2 build on CircleCI do display correctly.
| buildername='html') | ||
|
|
||
| app.build(False, []) | ||
| # need to build with this mode for automodule and backrefs to work |
There was a problem hiding this comment.
Not sure what this comment is about ... what do you mean by "this mode"? Is that related to moving app.build inside the with docutils_namespace()
There was a problem hiding this comment.
yes exactly, if the app.build is not in the context manager the links are not actually made
| assert 'numpy.arange.html' in lines | ||
| # assert '#module-matplotlib.pyplot' in lines | ||
| # assert 'pyplot.html' in lines | ||
| assert '#module-matplotlib.pyplot' in lines |
There was a problem hiding this comment.
Intersesting, I remember this was failing from time to time, but maybe now this is more stable?
There was a problem hiding this comment.
yeah the CIs are happy so I figure we can try it, if it ends up being inconsistent again we can comment it back out
| mod_file = op.join(out_dir, 'gen_modules', 'sphinx_gallery.sorting.html') | ||
| with codecs.open(mod_file, 'r', 'utf-8') as fid: | ||
| lines = fid.read() | ||
| assert 'FileNameSortKey' in lines |
There was a problem hiding this comment.
Very nice test!
Maybe add a few comments before each assert to explain a bit what they are checking? Proposal (improvements more than welcome!):
- first assert: "check that FileNameSortKey class appears in the reference API of the sphinx_gallery.sorting module"
- second assert: "check that the backreferences work when
Bonus points if you can add a test that the "normal" (i.e. in Python code) backreferences works? (maybe there is one already I haven't checked ...)
Yep completely agree, I was asking more out of curiosity! |
|
@lesteve comments addressed |
|
|
||
| Calls Sphinx-Gallery identify names function to figure out which | ||
| functions are called in the script and to which module do they belong. | ||
| This demonstrates how Sphinx-Gallery identifies names function to figure out |
There was a problem hiding this comment.
names function function names ?
| It uses both the code itself, as well as the docstrings (such as this one), | ||
| as adding a reference to :func:`numpy.sin` and :func:`numpy.exp` will create | ||
| proper backreferences even if they are not explicitly used. This is useful | ||
| in particular when functions return classes -- if you add them to the docstring |
There was a problem hiding this comment.
it seems from the regex below that this is true for any text block references yeah? Can we make this a bit clearer in the examples / documentation? They tend to refer to the docstring but this should also work within ####### blocks right?
|
this is a clever way to support functions that aren't directly imported! Just a couple quick thoughts but in general it LGTM. +1 on a merge once my question on whether this works in more than just example docstrings is addressed |
|
@lesteve it turns out NumPy intersphinx is complicated but I solved the linking issue by doing @choldgraf I have addressed your comments and done some rephrasing, can you see if this is better? |
Titan-C
left a comment
Titan-C
left a comment
There was a problem hiding this comment.
This looks great. +1 on my side.
I noticed a broken link in the docs, and have some irrelevant questions on the document style.
Yes Numpy and intersphinx is always a challenge, I agree just using obj because it works, other times I had to use other dark tricks. Most of the time is because this are C extensions.
| For more information on what files you can use, see `preparing your | ||
| repository <https://mybinder.readthedocs.io/en/latest/using.html#preparing-a-repository-for-binder>`_ | ||
| in the `Binder documentation <docs.mybinder.org>`_ for more information on | ||
| in the `Binder documentation <docs.mybinder.org>`__ for more information on |
There was a problem hiding this comment.
I'm wondering why you need to change the underscore? Trying to figure out(still ignorant), I realized this link is broken. Without http sphinx considers it as a local link.
There was a problem hiding this comment.
The _ makes it a link that can be used elsewhere, and it's named the same way so it creates a warning. The alternative would be just to not include the link in the second one, but this way makes it so that if one is removed both still work
| @@ -1,49 +1,52 @@ | |||
| :orphan: | |||
There was a problem hiding this comment.
Did sphinx complained this not being in any toctree? I missed that warning. But this makes me think if we should put this on our docs. Kind of reduce cognitive burden. All docs are online, but the release instructions are only in the repo.
Also why all the new white space?
There was a problem hiding this comment.
Did sphinx complained this not being in any toctree?
Yes I was getting rid of warnings
Also why all the new white space?
Getting rid of sphinx warnings about bad spacing (which are in master). But really these should all be part of the same bulleting indentation so it's a bugfix of sorts, too.
There was a problem hiding this comment.
We can add it to the docs later if someone is motivated, not sure there is much to gain (users don't care and maintainers can just look in the repo)
|
I'm planning to merge this once appveyor is happy (and if github isn't broken), speak now or forever hold your peace! |
|
actually it looks like appveyor did pass, but GitHub is just being wonky so the display isn't updating. I'm gonna merge since we've now got a bunch of +1s! 🎉 🎉 🎉 thanks @larsoner !!! 🎉 🎉 🎉 |
|
Nice! |
Adds to backreferences any function/class that gets
:func:or:meth:'ed in the docstring. This should be particularly useful for things likematplotlib.axes.Axesand similar classes that get instantiated only indirectly.Closes #205.