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

Workaround for dirhtml builder with wrong canonical URLs #1407

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

benjaoming
Copy link
Contributor

@benjaoming benjaoming commented Jan 11, 2023

Fix for #1406

  • Fix index.html being converted to index/

(will test manually since canonical URL does not seem to appear in local builds)

@benjaoming benjaoming changed the title Respect that dirhtml builder does not have .html in canonical URLs Workaround for dirhtml builder with wrong canonical URLs Jan 11, 2023
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.

This seems like a reasonable approach, but I'd like to see a test if we're going to implement this. Seems enough enough to have a test case that sets the canonical URL via sphinx?

@benjaoming
Copy link
Contributor Author

@ericholscher yes, agreed -- I would think that setting html_baseurl in conf.py is enough to mimic the intended behavior. LMK if you had other thoughts.

I don't think this needs to be a roadmap item for 1.2.0 but would be nice to publish it sooner or later.

@benjaoming
Copy link
Contributor Author

I'm going to leave this for a little while - but I don't think it's hard to add a test dimension in Tox that parameterizes conf.py in a similar fashion to how we are doing with -D writer=html4writer. It could be something like -D 'writer=dirhtml' -D 'html_baseurl=https://example.com' and then a verification that the canonical URL is set correctly in the output HTML.

@Blendify
Copy link
Member

@benjaoming
Copy link
Contributor Author

@Blendify this is just a workaround :) You'll find the upstream issue here: sphinx-doc/sphinx#9730

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

I think this is a good workaround for now. We should merge it and release a patch version.

sphinx_rtd_theme/layout.html Outdated Show resolved Hide resolved
@dmalan
Copy link

dmalan commented Apr 15, 2023

Thanks for this, @benjaoming, @humitos. This would indeed help with #1303 for now!

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.

None yet

5 participants