search

jdillard / sphinx-sitemap

Sphinx extension to generate a multi-lingual, multi-version sitemap for HTML builds
https://sphinx-sitemap.readthedocs.io/en/latest/index.html
MIT License
55 stars 22 forks source link

Forced URL scheme for multilanguage and multiversion #22

Closed mart-e closed 4 years ago

mart-e commented 4 years ago

Hello,

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

https://github.com/jdillard/sphinx-sitemap/blob/02bbe94613ca1164a0f73865659d240fadb3f808/sphinx_sitemap/__init__.py#L103-L104

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

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

Thanks

humitos commented 4 years ago

This assumption probably comes from Read the Docs, since this project was created after a discussion there https://github.com/readthedocs/readthedocs.org/issues/557#issuecomment-377420985

mart-e commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Great, thanks for the addition and for the adaptation !

jdillard commented 4 years ago

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.