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

docs: Make line-limited literalincludes less fragile #74

Closed
wants to merge 1 commit into from

Conversation

cassella
Copy link

@cassella cassella commented Jun 5, 2017

docs: Make line-limited literalincludes less fragile

Particularly, harden them against code motion.

Use :end-before: instead of line numbers to stop the full program
listings before the test routines. To this end, add a sentinel
comment line to the python programs where the literalinclude should
end.

Use the :pyobject: line-selection helper in the one case it applies --
the pure python quant(). It can't be used for the Chapel version
because it won't include the leading @chapel() with our current pinned
version of sphinx 1.3.6; fixed by sphinx-doc/sphinx#3348

The test_finance_chapel_numpy.py file that includes the @chapel()
quant is not included as a full file anywhere, only for this one
function and an import line, so use sentinel lines for those too.

@cassella
Copy link
Author

cassella commented Jun 5, 2017

There are more includes in usage.rst that this approach could apply to, but I wanted to see if the idea passes muster first.

@lydia-duncan
Copy link
Member

I'm in favor of having the line numbers grabbed be less fragile, and this seems like a reasonable approach to me. Does "begin-after" also base itself on a tag?

@cassella
Copy link
Author

cassella commented Jun 6, 2017

It does. I found that and the pyobject directive in
http://www.sphinx-doc.org/en/stable/markup/code.html

My first attempt at this used start-after and end-before for all the
bits of code inclusion. But then the literalincludes that include
nearly the whole file also include all those sentinel tags in-line. ☹️

Maybe with more effort, one could come up with sentinel tags that fit
naturally in the file.

I'm open to suggestion for a better tag to use -- that one was just
the first thing that came to mind as not going to be elsewhere in the
file, and self-explanatory to the reader and editor of the actual .py
files.

@lydia-duncan
Copy link
Member

What about having the start tag be the start of a documentation-style comment for the function? That way it fits in naturally in the whole file inclusion. The end section could be a "end of foo's definition" or something to that effect, that you might see normally on a longer function.

Of course, this would mean these tags vary from file to file, which could get a bit tedious.

@cassella
Copy link
Author

cassella commented Jun 6, 2017

That makes sense. The tags would need to vary anyway in order to include two separate bits from the same file.

@cassella
Copy link
Author

cassella commented Jun 6, 2017

Though on the other hand, since it's start-after, the line in the intro comment that's used as the tag won't itself be included in the targeted include. And it seems like the tag string would need to make it obvious to passers-by that it's being used externally, so please don't rephrase it without changing the docs too...

I wonder if the sphinx project would be amenable to having the pyobject directive include decorators like @Chapel().

@lydia-duncan
Copy link
Member

Hmm, true. There's probably a balance between "this looks too out of place to include in the export of the code" and "this is too subtle and so is vulnerable to tweaking". Perhaps one involving some checks to make sure that the tag is always present in the file? That might be something the Travis test should run . . .

@cassella
Copy link
Author

cassella commented Jun 6, 2017

Ah, the .travis.yml already has a "cd docs/ && make html". I believe I've already, err, tested, that that fails when the tag isn't found.

@lydia-duncan
Copy link
Member

I love it when we've already taken care of this stuff! Alright, how about you follow that path and get that all taken care of in this PR?

@cassella
Copy link
Author

cassella commented Jun 6, 2017

Ah, they were amenable to it in March: sphinx-doc/sphinx#3348

@cassella
Copy link
Author

Ooops, nope. If the start-after tag isn't found, nothing is included. If the end-before tag isn't found, it includes through the end of the file.

I'd thought that had caused the build to fail. Maybe I'd only said FAIL so loudly it seemed official.

I'm not sure what a Travis check for that case would look like. Most of the examples/ files wouldn't have the tags, and the tags wouldn't necessarily be the same in all of them. So I think it would either need to parse the literalinclude:: sections of the .rsts; or have its own metadata listing what files should have what tags, which would need to be kept up to date with the .rst files.

Unless there's a way to invoke sphinx such that it's stricter about this sort of thing? I can't find one via google or sphinx-build -h or the man page.

Particularly, harden them against code motion.

Use :end-before: instead of line numbers to stop the full program
listings before the test routines.  To this end, add a sentinel
comment line to the python programs where the literalinclude should
end.

Use the :pyobject: line-selection helper in the one case it applies --
the pure python quant().  It can't be used for the Chapel version
because it won't include the leading @chapel() with our current pinned
version of sphinx 1.3.6; fixed by sphinx-doc/sphinx#3348

The test_finance_chapel_numpy.py file that includes the @chapel()
quant is not included as a full file anywhere, only for this one
function and an import line, so use sentinel lines for those too.
@cassella
Copy link
Author

Cleaned it up a bit and squashed, as nearly every line in the original commit was changed.

@cassella
Copy link
Author

Of note, since test_finance_chapel_numpy.py was included only in snippets, I used tags for the include of the pyChapel version of quant() without having to worry about how they'd look when included.

@lydia-duncan
Copy link
Member

This seems relevant: sphinx-doc/sphinx#3108

@lydia-duncan
Copy link
Member

lydia-duncan commented Jun 13, 2017

We would need to update our Sphinx version to get that fix, which I can look into

@lydia-duncan
Copy link
Member

Wow, this lounged for a while. Can you remind me why we didn't merge it yet?

@cassella
Copy link
Author

I believe it was that the version of sphinx that the pychapel build uses won't report an error if the start-after or end-before tags don't match anything. The sphinx-doc issue you pointed out a few comments up covered adding that error reporting, so that the build will fail if those sentinel lines in the source are modified without updating the literalincludes in the .rst files.

And there's a good chance that once pychapel uses a sphinx with that feature, that new sphinx will also include the feature of the pyobject directive including the @Chapel decorator, so that the pyChapel quant case can use that instead. In that case I can update the PR before it's merged.

Assuming that the readthedocs build of the docs uses the new sphinx with the pyobject decorator support. I wonder if they've gotten the fixed line numbering layout yet.

@lydia-duncan
Copy link
Member

Pychapel has been abandoned, closing (shoulda done this earlier)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants