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

Forced URL scheme for multilanguage and multiversion #22

Closed
mart-e opened this issue Dec 26, 2019 · 6 comments
Closed

Forced URL scheme for multilanguage and multiversion #22

mart-e opened this issue Dec 26, 2019 · 6 comments

Comments

@mart-e
Copy link
Contributor

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

Hello,

Reading the code, I understand that you assume the page is published, using the version and language configuration, following the scheme :

  • https://<html_baseurl>/<language>/<version>/<page>.html

ET.SubElement(url, "loc").text = site_url + \
app.builder.config.language + '/' + version + link

Is there a reason to force this construction? For instance, on our website, we use the construction:

  • https://<base>/<version>/<language>/<page>/html

What would be the way to make it work for our scheme?

Thanks

@humitos

This comment has been minimized.

Copy link

@humitos humitos commented Dec 26, 2019

This assumption probably comes from Read the Docs, since this project was created after a discussion there readthedocs/readthedocs.org#557 (comment)

@mart-e

This comment has been minimized.

Copy link
Contributor Author

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

Thanks for the reply.

@jdillard Would you accept a patch for a configurable url? For instance something like:

scheme = app.config.i18n_url_scheme or "/{lang}/{version}/{link}"
url = site_url + scheme.format(lang=app.builder.config.language, version=version, link=link)
@jdillard

This comment has been minimized.

Copy link
Owner

@jdillard jdillard commented Dec 29, 2019

Correct, I had made it opinionated based on RTD's scheme just to get something out. Although, it looks like I forgot to make an issue to remind myself to circle back and fix it, nice catch and nice fix!

PR's gladly accepted! I just did a quick test and that looks like that solution will work, it just may need some finagling to get everything aligned again. If you want to submit a first draft I can help clean it up if it needs any.

@jdillard

This comment has been minimized.

Copy link
Owner

@jdillard jdillard commented Feb 19, 2020

I merged your changes (and mine) in and then realized it forced a multilanguage and multiversion scheme as a default and didn't like that so changed the behavior to only include language and version if they exist and auto appended a trailing slash to each. You can still re-arrange them, you just don't need slashes anymore. I also changed the new config value name to sitemap_url_scheme since language is optional now.

That is all documented in the README now if you would like to test the master branch before I cut a new version.

@mart-e

This comment has been minimized.

Copy link
Contributor Author

@mart-e mart-e commented Feb 19, 2020

Great, thanks for the addition and for the adaptation !

jdillard added a commit that referenced this issue Feb 19, 2020
@jdillard

This comment has been minimized.

Copy link
Owner

@jdillard jdillard commented Feb 20, 2020

Thanks for the contribution, this was a great step forward! I went ahead and released it as version 2.0.0 since it has the possibility to be a breaking change for lower versions.

I already tested it with some unique schemes I have in other projects and the configuration is much cleaner now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.