search

spatialaudio / nbsphinx

:ledger: Sphinx source parser for Jupyter notebooks
https://nbsphinx.readthedocs.io/
MIT License
449 stars 128 forks source link

DOC: Add download link for executed .ipynb file #751

Closed mgeier closed 1 year ago

mgeier commented 1 year ago

See discussion in #745.

agriyakhetarpal commented 1 year ago

Looks good to me. Continuing the discussion from the issue: I don't think target="_blank" would be needed if we already have download, because it is used for opening links in a different tab.

If you decide to document a button or link for downloading notebooks as a feature as well (and thereby close the mentioned issue) I would also suggest mentioning about how downloads of this manner will not work on cross-origin links, e.g., on PR builds on readthedocs because the URL would not be the same as that of the stable documentation. The reason is that it is a security measure in modern browsers (the origin of which was discussed in this Bugzilla report): a cross-origin download link could lead to users downloading an unverified file of the same name hosted on a different server with malicious intent.

mgeier commented 1 year ago

If you decide to document a button or link for downloading notebooks as a feature as well (and thereby close the mentioned issue)

What do you mean by "document"? I'm suggesting here to add links to the docs, are you talking about some additional documentation?

I would also suggest mentioning about how downloads of this manner will not work on cross-origin links, e.g., on PR builds on readthedocs because the URL would not be the same as that of the stable documentation.

But isn't the .ipynb file always in the same domain as the HTML? It's right next to it, in the same directory, isn't it?

The download link also works on the PR build on RTD: https://nbsphinx--751.org.readthedocs.build/en/751/

agriyakhetarpal commented 1 year ago

What do you mean by "document"? I'm suggesting here to add links to the docs, are you talking about some additional documentation?

Yes, if you decide to add some additional documentation for how others can use this feature, that is

But isn't the .ipynb file always in the same domain as the HTML? It's right next to it, in the same directory, isn't it?

The download link also works on the PR build on RTD: https://nbsphinx--751.org.readthedocs.build/en/751/

That's strange. It does not work on the PR build where I had added it myself. Maybe it is due to the extra target="_blank" attribute (your method contains just the download attribute)

Edit: okay, I see it now. In your case, it downloads the notebook from the PR build itself, but our build leads us to a different link (it downloads the notebook from docs.pybamm.org instead). So the point about cross-origin links remains valid.

mgeier commented 1 year ago

In your case, it downloads the notebook from the PR build itself, but our build leads us to a different link (it downloads the notebook from docs.pybamm.org instead). So the point about cross-origin links remains valid.

Yeah, but that's definitely not a workflow that I would recommend.

You are linking to the latest downloads, and those links will likely become invalid over time.

I would recommend to link to the notebooks of the exact same version as the doc build. This should be easy, because those notebooks are created by nbsphinx anyway. No need to worry about cross-origin links.

agriyakhetarpal commented 1 year ago

That sounds like a good idea to prevent potential link rot. Thank you for the recommendation!