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

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

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

mgeier
Copy link
Member

@mgeier mgeier commented Jun 29, 2023

See discussion in #745.

@mgeier mgeier mentioned this pull request Jun 29, 2023
@agriyakhetarpal
Copy link

agriyakhetarpal commented Jun 30, 2023

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
Copy link
Member Author

mgeier commented Jul 6, 2023

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
Copy link

agriyakhetarpal commented Jul 6, 2023

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
Copy link
Member Author

mgeier commented Jul 7, 2023

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.

@mgeier mgeier merged commit 55c4809 into spatialaudio:master Jul 7, 2023
2 checks passed
@mgeier mgeier deleted the doc-download-ipynb branch July 7, 2023 18:38
@agriyakhetarpal
Copy link

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

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