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

Read the Docs: handle special URLs #200

Merged
merged 1 commit into from
Jul 13, 2022
Merged

Conversation

humitos
Copy link
Member

@humitos humitos commented Jul 12, 2022

Read the Docs serves their static files under special URLs starting with /_/
that should not be modified by this extension. Otherwise, the flyout and other
integrations are broken.

This commit checks if the documentation is building on Read the Docs and if the
URLs starts with /_/ and skip the manipulation in that case.

404 page showing the flyout properly: https://sphinx-notfound-page--200.org.readthedocs.build/en/200/notfound

@humitos humitos requested a review from a team as a code owner July 12, 2022 08:45
@humitos humitos requested a review from stsewd July 12, 2022 08:45
@humitos humitos force-pushed the humitos/readthedocs-special-urls branch from 10b16ed to f4b3537 Compare July 12, 2022 09:26
@humitos humitos self-assigned this Jul 12, 2022
@humitos humitos force-pushed the humitos/readthedocs-special-urls branch from f4b3537 to d239688 Compare July 12, 2022 09:35
Read the Docs serves their static files under special URLs starting with `/_/`
that should not be modified by this extension. Otherwise, the flyout and other
integrations are broken.

This commit checks if the documentation is building on Read the Docs and if the
URLs starts with `/_/` and skip the manipulation in that case.
@humitos humitos force-pushed the humitos/readthedocs-special-urls branch from d239688 to 4eb39e7 Compare July 12, 2022 09:47
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

Looks reasonable 👍

if resource and '://' in otheruri:
# allow non-local resources given by scheme
return otheruri

if READTHEDOCS and otheruri.startswith('/_/'):
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to special case RTD here, or should we just not have this run on any URL that is already absolute? I don't fully understand the issue, but generally absolute URL's won't need to change?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a good question. I thought we have already thought about this but I didn't find a test or discussion on the issues.

I think it should be fine to not modify absolute URLs, but I'd prefer to not introduce it right now just in case I'm missing something. So, I created an issue to track, discuss and think more about this before moving forward: #206

@humitos humitos mentioned this pull request Jul 13, 2022
@humitos humitos merged commit 1b735e3 into main Jul 13, 2022
@humitos humitos deleted the humitos/readthedocs-special-urls branch July 13, 2022 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants