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

Rewrite README links in notebooks to point to Sphinx index files #1401

Merged
merged 1 commit into from Apr 30, 2020

Conversation

huonw
Copy link
Member

@huonw huonw commented Apr 29, 2020

Our demo notebooks contain links to READMEs. These links don't work on Read the Docs/in Sphinx, because we have index (per the web server convention) files for each directory, not READMEs.

This PR writes a basic sphinx extension that rewrites any relative links to a file called "README.md" to instead be a link to "index.txt", matching our Sphinx index files. I modelled this after:

This approach allows us to retain links to READMEs in the notebooks, so that they work locally and on Github. These links are probably useful even with the move to Read the Docs being the canonical display of demos (#1391), due to being useful locally. (A simpler approach that breaks this would be to change the links to be correct for Read the Docs within each notebook.)

This also adjusts the case of one of the connector/neo4j README file, so that it's consistent with the rest of the repo, to make this code simpler.

This fixes 12 out of 66 of the remaining warnings in #1360; all of the ones like:

WARNING: File not found: 'demos/README.md'
WARNING: File not found: 'demos/node-classification/README.md'

Example: in the GCN node classification notebook (https://github.com/stellargraph/stellargraph/blob/1cca6b6c6f4cd1fea7c6932918cba18d15eef6d4/demos/node-classification/gcn/gcn-cora-node-classification-example.ipynb), there's a conclusion that includes links to both the parent node-classification README and the all notebooks README. Its Markdown source is:

StellarGraph includes [other algorithms for node classification](../README.md) and [algorithms and demos for other tasks](../../README.md).

Before: https://stellargraph.readthedocs.io/en/latest/demos/node-classification/gcn/gcn-cora-node-classification-example.html#Conclusion These links stayed pointing to README.md in the respective parent directories, which don't exist in the Sphinx docs/on Read the Docs, and so they're dead links. Generated HTML (note the external CSS class, and the href=.../README.md):

StellarGraph includes <a class="reference external" href="../README.md">other algorithms for node classification</a> and <a class="reference external" href="../../README.md">algorithms and demos for other tasks</a>.

After: https://stellargraph--1401.org.readthedocs.build/en/1401/demos/node-classification/gcn/gcn-cora-node-classification-example.html#Conclusion The links were rewritten and now work. Generated HTML (note the internal CSS class, and the href=.../index.html):

StellarGraph includes <a class="reference internal" href="../index.html"><span class="doc">other algorithms for node classification</span></a> and <a class="reference internal" href="../../index.html"><span class="doc">algorithms and demos for other tasks</span></a>.

See: #1292

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB.

@codeclimate
Copy link

codeclimate bot commented Apr 29, 2020

Code Climate has analyzed commit e319234 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Security 1

View more on Code Climate.

@huonw huonw marked this pull request as ready for review April 29, 2020 06:55
@huonw huonw requested review from kjun9 and timpitman April 29, 2020 06:56
Copy link
Contributor

@kjun9 kjun9 left a comment

Choose a reason for hiding this comment

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

Nice 👍

huonw added a commit that referenced this pull request Apr 30, 2020
…1398)

The raw HTML table was not converting to reStructuredText through `nbsphinx`
(which, I think, uses `jupyter nbconvert --to=rst`, which uses pandoc). The
output lost the links + images (see more details about `pandoc` in #1398).

This PR updates the links to be normal markdown links + images, which ensures
they do get converted correctly. To distinguish these links from normal body
text, these are formatted as a block-quote (`> ...`). This gives acceptable (not
great) formatting on each of the major platforms one might be using to look at
this (see images in #1398):

- Jupyter locally
- Github:
  https://github.com/stellargraph/stellargraph/blob/2972637ec3b75c7ab7bc6a7a96697489b03f62fd/demos/basics/loading-networkx.ipynb
- nbviewer:
  https://nbviewer.jupyter.org/github/stellargraph/stellargraph/blob/2972637ec3b75c7ab7bc6a7a96697489b03f62fd/demos/basics/loading-networkx.ipynb
- Read the Docs:
  https://stellargraph--1398.org.readthedocs.build/en/1398/demos/basics/loading-networkx.html


Note that both Github and nbviewer have CSS that forces images to be formatted
as block elements, and so they're positioned separately (instead of inline
elements as is the default). This is unfortunate, but per #1391, we're moving
towards Read the Docs being the main online display of our demos.

We can potentially improve the Read the Docs formatting with custom CSS or some
post-processing (similar to #1401).

See: #1293
Copy link
Contributor

@timpitman timpitman left a comment

Choose a reason for hiding this comment

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

Looks from https://readthedocs.org/api/v2/build/10932108.txt that this removed a bunch of warnings of bad links 👍

@huonw huonw merged commit e60f354 into develop Apr 30, 2020
@huonw huonw deleted the bugfix/1292-rewrite-readme-links branch May 20, 2020 05:28
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.

None yet

3 participants