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

fix: broken github link on generated documentation pages #12871

Merged
merged 9 commits into from
Apr 19, 2021

Conversation

cocobennett
Copy link
Contributor

@cocobennett cocobennett commented Mar 25, 2021

Fix #11919

Currently trying to use the fix listed on sphinx-doc/sphinx#2386

Copy link
Member

@MrMino MrMino left a comment

Choose a reason for hiding this comment

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

Hmm... this is taken from https://github.com/readthedocs/sphinx_rtd_theme/blob/master/sphinx_rtd_theme/breadcrumbs.html, is it?

Looks like this is already inside sphinx-rtd-theme - why does it have to be added separately?

@cocobennett
Copy link
Contributor Author

@MrMino Thanks, you're right. I just need to figure out how to make sure to only show the link if the url is valid

@cocobennett
Copy link
Contributor Author

Unfortunately, I have no idea how to verify or test the documentation works here in the UI or locally, so I'm closing for now. :(

@cocobennett cocobennett deleted the github-link-fix branch April 11, 2021 18:18
@MrMino
Copy link
Member

MrMino commented Apr 11, 2021

@cocobennett Don't hesitate to ask if something is unclear 🙂. You can generate the docs locally by issuing the following commands from the repository root:

pip install -r docs/requirements.txt pytest nose testpath
cd docs/
make html

Afterwards you'll find the index page of the docs under docs/build/html/index.html.

@cocobennett cocobennett restored the github-link-fix branch April 12, 2021 18:39
@cocobennett cocobennett reopened this Apr 12, 2021
@cocobennett cocobennett marked this pull request as ready for review April 12, 2021 18:40
@cocobennett
Copy link
Contributor Author

cocobennett commented Apr 12, 2021

@MrMino My last commit removes the link for all pages. It looks like the link can't be removed for only generated pages until readthedocs/sphinx_rtd_theme#252 is fixed. What is your recommendation?

@cocobennett
Copy link
Contributor Author

Hey there, following up. Does anyone have a recommendation here?

@MrMino
Copy link
Member

MrMino commented Apr 16, 2021

@cocobennett Sorry, got distracted and forgot to respond. Could you please summarize what is going on on the sphinx-rtd-theme side? The info in these issues is reliant on previous knowledge about the internals of RTD/theme, and I don't have much of it.

@Carreau
Copy link
Member

Carreau commented Apr 17, 2021

Oh, look like you added PNG as part of b8ade5c, I've amended your last commit.

I think we can merge this and see how it's behaving on readthedocs and iterate.

Before merging and just to make sure, it looks like your first commit is referencing a different email than the others (potentially work email), and another have a different name, I want to make sure you are not non intentionally revealing private informations, and that this is ok.

let us know if you need help amending the authoring informations.

@cocobennett
Copy link
Contributor Author

@cocobennett Sorry, got distracted and forgot to respond. Could you please summarize what is going on on the sphinx-rtd-theme side? The info in these issues is reliant on previous knowledge about the internals of RTD/theme, and I don't have much of it.

It looks like the issue of telling the difference between a generated page that will lead to a 404 vs an actual page has to be fixed first within sphinx-rtd-theme before anything can be done here to fix, or at least that's what I've gathered

@cocobennett
Copy link
Contributor Author

cocobennett commented Apr 17, 2021

Oh, look like you added PNG as part of b8ade5c, I've amended your last commit.

I think we can merge this and see how it's behaving on readthedocs and iterate.

Before merging and just to make sure, it looks like your first commit is referencing a different email than the others (potentially work email), and another have a different name, I want to make sure you are not non intentionally revealing private informations, and that this is ok.

let us know if you need help amending the authoring informations.

Yes! That was from a different account before I re-configured my local setup. I am okay with it staying in. Thanks for the PNG fix and for asking :)

@cocobennett
Copy link
Contributor Author

@cocobennett Sorry, got distracted and forgot to respond. Could you please summarize what is going on on the sphinx-rtd-theme side? The info in these issues is reliant on previous knowledge about the internals of RTD/theme, and I don't have much of it.

It looks like the issue of telling the difference between a generated page that will lead to a 404 vs an actual page has to be fixed first within sphinx-rtd-theme before anything can be done here to fix, or at least that's what I've gathered

Well, it looks like it was fixed in the theme, so I don't know why I can't get it to tell the difference https://github.com/readthedocs/sphinx_rtd_theme/pull/393/files

@cocobennett
Copy link
Contributor Author

Trying suggestion here sphinx-doc/sphinx#2386 (comment)

I tested it locally, but seems to act differently when generated in the PR, so trying both

@cocobennett cocobennett marked this pull request as draft April 17, 2021 01:53
@Carreau
Copy link
Member

Carreau commented Apr 18, 2021

I tested it locally, but seems to act differently when generated in the PR, so trying both

Let's try to merge and see how it affects the doc; I know that readthedocs sometime use a different version of the theme. So that may be why.

If you convert it back to non-draft I'm happy to merge this.

@cocobennett cocobennett marked this pull request as ready for review April 18, 2021 20:51
@Carreau Carreau merged commit f1f44b5 into ipython:master Apr 19, 2021
@Carreau
Copy link
Member

Carreau commented Apr 19, 2021

Thanks ! Let's see how it renders on readthedocs in a couple of hours.

@MrMino
Copy link
Member

MrMino commented Apr 19, 2021

I don't think this fixed the problem, unfortunately. The links that #11919 mentions are still visible & broken.

@Carreau Carreau added this to the 8.0 milestone Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken link to "Edit on Github" in ipython documentation
3 participants