Fix download role

[chrisjsewell]

This PR makes jupyter-download act equivalently to the standard download role and fixes a bug when supplying relative paths.
See the added test for an example of how this works, but basically you can now include alternative caption text: :jupyter-download:notebook:`alternative text <../path/to/a/notebook>`.
I also added jupyter-download:nb as an alternative name, because jupyter-download:notebook is a mouthful!

[chrisjsewell]

err sphinx.utils.docutils.ReferenceRole isn't available until sphinx v2, @akhmerov do we really need to still support sphinx 1.8, given that sphinx v3 is now out?
If so, I'm just going to have to copy over a bunch of code from sphinx 2 to here

[akhmerov]

I'm hesitant to bump the version: 1.8 is about 2 years ago, and 2.0 is about 1 year: that's a bit on the short side. This should be weighed against the extra work of course. the role itself doesn't seem like a lot of code, or does it drag a lot with it?

[chrisjsewell]

well it would be the inherited class above as well: https://github.com/sphinx-doc/sphinx/blob/e4c3e24e85a6e32e3f50bc506e895eb356f0ab12/sphinx/util/docutils.py#L342-L435,
I think thats it

[akhmerov]

Thanks for fixing this! Other than the above comments, all looks good!

[chrisjsewell]

Thanks for fixing this! Other than the above comments, all looks good!

Cheers, do you want me to copy across the sphinx 2 code into a separate module here then, and move the sphinx requirement back to 1.8?

[akhmerov]

It's a tough call:

• Porting code from sphinx would also properly require copying the license, creating a separate file for it and keeping track of removing this at a later point.
• On the other hand this is a bugfix, and therefore if any user that relies on sphinx 1.8 encounters this bug, they wouldn't get the fix.

Ultimately I think bumping sphinx is OK: this repo doesn't have a lot of workforce, and fresher dependencies = less maintenance.

Still, I'd like to ask for two extra fixes:

• Please mention the new functionality in the docs
• Please update the sphinx version in requirements.txt, or we end up with a broken rtd build.

[chrisjsewell]

Still, I'd like to ask for two extra fixes

done in db05e13 👍

[chrisjsewell]

dammit the link is not being created for some reason: https://51-73843754-gh.circle-artifacts.com/0/html/index.html#downloading-the-code-as-a-script, this is definitely working locally, I'll look into it...

[chrisjsewell]

I altered your circle-ci setup, to fail on warnings (like we have in executablebooks).
So its failing, because its trying to find the download file before it is created: https://app.circleci.com/pipelines/github/jupyter/jupyter-sphinx/54/workflows/7084890e-eaa2-4ee5-8fc8-31bce8534ee5/jobs/53/steps

Maybe we should check if this has ever actually worked with sphinx >=2, because I don't see how the changes I have made would have affected this? Perhaps there is some change in sphinx 2, altering the order of processes?

Note: using this with myst-nb, this code works, because we execute and copy the notebook to the build folder at the start, rather than in a transform, as done in jupyter-sphinx. So the file is there to be found.

[chrisjsewell]

hmm there doesn't seem to be any differences. I just don't see how this was working before, given that the dowloadable documents are processed at read time, before the notebooks/scripts are actually created in the transform:

• https://github.com/sphinx-doc/sphinx/blob/16533cff9a1e20fc10ec78eaf7c4796cd3552965/sphinx/environment/collectors/__init__.py#L37
• https://github.com/sphinx-doc/sphinx/blob/16533cff9a1e20fc10ec78eaf7c4796cd3552965/sphinx/environment/collectors/asset.py#L126

[akhmerov]

Ugh, it was likely broken by #107.

[chrisjsewell]

The only easy (but hacky) fix I can think of is to "touch" the file, so the DownloadCollector still notes the file. The other way would be to use a custom node type, then do some transform/post-trasform to find those nodes, add the paths to the collector and change them back to the right node type. Thats probably not ideal either

[chrisjsewell]

fixed it! grr, it was actually to do with the path normalisations; sphinx fails if there are any .. in the final path to copy, but if you normalise \..\path\to\..\something, you get \path\something, whereas we want \..\path\something. Hence the change in 510f548

Anyway it all works now

[akhmerov]

Phew, that's a relief. Good to know that it isn't the transform that breaks stuff.

Something is broken with logging in to circleci for me (complete with 500 errors from github)—can you please share a link to the build artifacts?

[chrisjsewell]

https://59-73843754-gh.circle-artifacts.com/0/html/index.html

[akhmerov]

thanks, all looks good!

[chrisjsewell]

thanks @akhmerov, perhaps a new release is in order 😬 ? (I guess bumping to v0.3.0, since we dropped sphinx 1.8 compatibility)

[akhmerov]

I'd like to look at #126 before that, because it breaks RTD. #134 is also an unfortunate consequence of #107, which I hope to fix before making another release.

[chrisjsewell]

Sounds fair, would you mind doing a quick alpha release though (as we did before), so I can use it in my repository?

[akhmerov]

Will do once I have some time (dense schedule with exams). Or @choldgraf if you could do that, it'd be also great!

[choldgraf]

I just cut https://github.com/jupyter/jupyter-sphinx/releases/tag/v0.3.0a1 which I think will push the alpha release? @chrisjsewell lemme know if I messed something up

[choldgraf]

boom: https://pypi.org/project/jupyter-sphinx/0.3.0a1/

[chrisjsewell]

Cheers will do