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

[IMP] allow custom URL scheme for internationalized pages #23

Merged
merged 3 commits into from Feb 19, 2020

Conversation

mart-e
Copy link
Contributor

@mart-e mart-e commented Dec 30, 2019

Allowing to use a custom i18n_url_scheme parameter.
Before this commit, the URLs were forced to use the format
<site>/<lang>/<version>/<link> which is the format used by RTD but
some may use a different one.

Fixes #22

Allowing to use a custom i18n_url_scheme parameter.
Before this commit, the URLs were forced to use the format
<site>/<lang>/<version>/<link> which is the format used by RTD but
some may use a different one.
@mart-e
Copy link
Contributor Author

mart-e commented Dec 30, 2019

@jdillard there you go.
This needs to be triple checked for the leading/trailing /. For instance, I am not sure about

if app.builder.config.version:
version = app.builder.config.version + '/'
else:
version = app.builder.config.version

meaning the version will always ends with a / ?

Should the site_url also always end with a slash?

edit: added some sanity check in the second commit

I would also like the opinion of my colleague @xmo-odoo before merging this to see if it integrates well with our deployment at odoo/documentation#499

In case somebody does not respect the tacit rule of ending site_url or
version by a slash or not
@xmo-odoo
Copy link

xmo-odoo commented Jan 6, 2020

Seems fine though the handling of trailing slashes seems like a bit of a mess. Maybe urllib.parse.urljoin could help there? It should handle trailing / fine, though it requires components to not have leading / as that's an "absolute" link:

>>> urljoin('http://foo.com/bar/baz/', 'qux/')
'http://foo.com/bar/baz/qux/'
>>> urljoin('http://foo.com/bar/baz/', '/qux/')
'http://foo.com/qux/'

@jdillard
Copy link
Owner

I don't know what was up with the handling of version you linked to. I changed how it was handled in d611027.

This defaults the scheme to "{lang}/{version}/{link}" where {lang} is defaulted to "en" (I didn't want to default to a specific language, but it was easiest) and {version} is defaulted to "latest". If the user doesn't want any of those defaults they can change them, such as setting the scheme to "{version}/{link}" or even just "{link}". Does that change make sense?

Copy link
Contributor Author

@mart-e mart-e left a comment

Choose a reason for hiding this comment

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

Thanks for the update, it should be compatible with our deployment, this way, we will be able to use sphinx-sitemap 👍

sphinx_sitemap/__init__.py Show resolved Hide resolved
@jdillard jdillard merged commit 6d5e7b4 into jdillard:master Feb 19, 2020
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.

Forced URL scheme for multilanguage and multiversion
3 participants