-
Notifications
You must be signed in to change notification settings - Fork 28
docs: create underscored versions of every page as a redirect target #540
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
base: main
Are you sure you want to change the base?
Changes from all commits
5fc3d99
60cbc6b
1b04635
12424c5
ac587a8
8d987c3
d06a605
8ddcba3
de420e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,106 @@ | ||
| # Copyright 2025 Canonical Ltd. | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| """Populate ``rediraffe_redirects`` with underscore/hyphen separator variants.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import typing | ||
|
|
||
| if typing.TYPE_CHECKING: | ||
| import sphinx.application | ||
| import sphinx.environment | ||
|
|
||
|
|
||
| def setup(app: sphinx.application.Sphinx) -> dict[str, str | bool]: | ||
| """Sphinx extension entrypoint -- register the ``env-updated`` hook.""" | ||
| app.connect('env-updated', _automatic_redirects) | ||
| return {'version': '1.0.0', 'parallel_read_safe': True, 'parallel_write_safe': True} | ||
|
|
||
|
|
||
| def _automatic_redirects( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this the best name? It feels like most of what it's doing is validation. |
||
| app: sphinx.application.Sphinx, env: sphinx.environment.BuildEnvironment | ||
| ) -> None: | ||
| # Don't create redirects during the per-package passes. | ||
| if app.config.package is not None: | ||
| return | ||
| redirects = _build_redirects(set(env.found_docs)) | ||
| target = typing.cast('dict[str, str]', app.config.rediraffe_redirects) | ||
| assert isinstance(target, dict), f'rediraffe_redirects must be a dict, not {target}' | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the complaint is the type, do you think it would be clearer to have the type in the error rather than whatever the str is? Or even just the repr maybe? |
||
| if not set(target).isdisjoint(redirects.keys()): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems odd that one of these implicitly uses "iter dict is keys" and the other has it explicit. Is that to satisfy some type check? |
||
| lines = ['Manually configured redirects would be clobbered by automatic redirects:'] | ||
| lines.extend( | ||
| f'{k} -> {target[k]} would be overwritten by {k} -> {v}' | ||
| for k, v in redirects.items() | ||
| if k in target | ||
| ) | ||
| raise ValueError('\n - '.join(lines)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't love a multi-line message in a ValueError. Where does this show up, in the build output? Do you have an example of what that looks like? If it looks good and is the only place it would be expected, I would be convinced I think. Same for the ones below. |
||
| if not set(target.values()).isdisjoint(redirects.keys()): | ||
| lines = [ | ||
| 'The following manually configured redirects would chain with automatic redirects:' | ||
| ] | ||
| lines.extend( | ||
| f'{k} -> {v} should point to {redirects[v]}' | ||
| for k, v in target.items() | ||
| if v in redirects | ||
| ) | ||
| raise ValueError('\n - '.join(lines)) | ||
| if not set(target).isdisjoint(redirects.values()): | ||
| # This is probably safe in practice, but we'd like to notice the first time it happens. | ||
| lines = ['The following automatic redirects chain with manually configured redirects:'] | ||
| lines.extend(f'{k} -> {v} -> {target[v]}' for k, v in redirects.items() if v in target) | ||
| raise ValueError('\n - '.join(lines)) | ||
| target.update(redirects) | ||
|
|
||
|
|
||
| def _build_redirects(found_docs: set[str]) -> dict[str, str]: | ||
| """Redirect underscored names to hyphenated ones and vice versa. | ||
|
|
||
| Categories area also separately aliased with both variants and with no separator. | ||
| """ | ||
| redirects: dict[str, str] = {} | ||
| for docname in sorted(found_docs): | ||
| category, _, doc = docname.partition('/') | ||
| category_variants = { | ||
| category, | ||
| category.replace('-', '_'), # how_to | ||
| category.replace('_', '-'), # how-to | ||
| category.replace('_', '').replace('-', ''), # howto | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean it's not just "how-to" -> "howto", but also all other I understand the motivation for hyphen/underscore redirects, but I'm less sure about removing them. Is this an issue we see in practice? |
||
| } | ||
| for category_variant in sorted(category_variants): | ||
| for doc_variant in sorted({doc, _separator_variant(doc)}): | ||
| if (category_variant, doc_variant) == (category, doc): | ||
| continue | ||
| alias = f'{category_variant}/{doc_variant}' | ||
| assert alias not in found_docs, f'Alias {alias} is a real page!' | ||
| assert alias not in redirects, ( | ||
| f'Alias {alias} already redirects to {redirects[alias]}' | ||
| ) | ||
| if docname.endswith('/index.html'): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to handle the llms.txt markdown files here too? |
||
| # Remove 'index.html', preserve trailing '/'. | ||
| redirects[alias] = docname.removesuffix('index.html') | ||
| else: | ||
| redirects[alias] = docname | ||
| return redirects | ||
|
|
||
|
|
||
| def _separator_variant(docname: str) -> str: | ||
| """Return the docname with ``-`` and ``_`` swapped.""" | ||
| if '-' in docname: | ||
| assert '_' not in docname, f"Docname {docname} should not contain both '-' and '_'" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel this ought to be a ValueError not an assertion. Also below. |
||
| return docname.replace('-', '_') | ||
| if '_' in docname: | ||
| assert '-' not in docname, f"Docname {docname} should not contain both '-' and '_'" | ||
| return docname.replace('_', '-') | ||
| return docname | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,168 @@ | ||||||
| # Copyright 2025 Canonical Ltd. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| # | ||||||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||||||
| # you may not use this file except in compliance with the License. | ||||||
| # You may obtain a copy of the License at | ||||||
| # | ||||||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||||||
| # | ||||||
| # Unless required by applicable law or agreed to in writing, software | ||||||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||||||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||
| # See the License for the specific language governing permissions and | ||||||
| # limitations under the License. | ||||||
|
|
||||||
| # ruff: noqa: D103 (function docstrings) | ||||||
|
|
||||||
| """Unit tests for the `automatic_redirects` local Sphinx extension.""" | ||||||
|
|
||||||
| from __future__ import annotations | ||||||
|
|
||||||
| import types | ||||||
| import typing | ||||||
|
|
||||||
| import automatic_redirects | ||||||
| import pytest | ||||||
|
|
||||||
| if typing.TYPE_CHECKING: | ||||||
| import sphinx.application | ||||||
|
|
||||||
|
|
||||||
| class TestSeparatorVariant: | ||||||
| @pytest.mark.parametrize( | ||||||
| ('docname', 'expected'), | ||||||
| [ | ||||||
| # Hyphens are translated to underscores and vice versa. | ||||||
| ('---', '___'), | ||||||
| ('___', '---'), | ||||||
| # / isn't special for this function. | ||||||
| ('how-to/manage-libraries', 'how_to/manage_libraries'), | ||||||
| ('how_to/manage_libraries', 'how-to/manage-libraries'), | ||||||
| # If there are no separators, the value is returned unmodified. | ||||||
| ('noseparators', 'noseparators'), | ||||||
| ('', ''), | ||||||
| ], | ||||||
| ) | ||||||
| def test_ok(self, docname: str, expected: str): | ||||||
| variant = automatic_redirects._separator_variant(docname) | ||||||
| assert variant == expected | ||||||
|
|
||||||
| def test_mixed_separators_raises(self): | ||||||
| """A docname containing both separators is an error.""" | ||||||
| with pytest.raises(AssertionError, match='should not contain both'): | ||||||
| automatic_redirects._separator_variant('-_-') | ||||||
|
|
||||||
|
|
||||||
| class TestBuildRedirects: | ||||||
| @pytest.mark.parametrize( | ||||||
| ('found', 'expected'), | ||||||
| [ | ||||||
| # No separators -> no redirects. | ||||||
| ({'foo', 'bar'}, {}), | ||||||
| # Hyphen -> underscore and vice versa. | ||||||
| ({'foo/a-b'}, {'foo/a_b': 'foo/a-b'}), | ||||||
| ({'foo/a_b'}, {'foo/a-b': 'foo/a_b'}), | ||||||
| # Redirects are created for category variants. | ||||||
| ({'how-to/foo'}, {'how_to/foo': 'how-to/foo', 'howto/foo': 'how-to/foo'}), | ||||||
| # Trailing /index.html is handled. | ||||||
| ({'foo/a-b/index.html'}, {'foo/a_b/index.html': 'foo/a-b/'}), | ||||||
| # Redirects are created from original name with category variant. | ||||||
| ( | ||||||
| {'how-to/a-b/index.html'}, | ||||||
| { | ||||||
| 'how-to/a_b/index.html': 'how-to/a-b/', | ||||||
| 'how_to/a-b/index.html': 'how-to/a-b/', | ||||||
| 'how_to/a_b/index.html': 'how-to/a-b/', | ||||||
| 'howto/a-b/index.html': 'how-to/a-b/', | ||||||
| 'howto/a_b/index.html': 'how-to/a-b/', | ||||||
| }, | ||||||
| ), | ||||||
| ], | ||||||
| ) | ||||||
| def test_ok(self, found: set[str], expected: dict[str, str]): | ||||||
| redirects = automatic_redirects._build_redirects(found) | ||||||
| assert sorted(redirects.items()) == list(expected.items()) | ||||||
|
|
||||||
| def test_variant_as_real_page_raises(self): | ||||||
| # When both separator variants are real pages, the generated alias would | ||||||
| # collide with a real page -- a configuration error that should fail | ||||||
| # loudly rather than silently shadowing a page. | ||||||
| found = {'how-to/foo', 'how_to/foo'} | ||||||
| with pytest.raises(AssertionError, match='is a real page'): | ||||||
| automatic_redirects._build_redirects(found) | ||||||
|
|
||||||
|
|
||||||
| class TestAutomaticRedirects: | ||||||
| @staticmethod | ||||||
| def fake_app( | ||||||
| *, | ||||||
| package: str | None = None, | ||||||
| rediraffe_redirects: dict[str, str] | str | None = None, | ||||||
| found_docs: typing.Iterable[str] = (), | ||||||
| ) -> sphinx.application.Sphinx: | ||||||
| """Build a minimal fake Sphinx app/config/env for ``_automatic_redirects()``.""" | ||||||
| config = types.SimpleNamespace(package=package, rediraffe_redirects=rediraffe_redirects) | ||||||
| env = types.SimpleNamespace(found_docs=set(found_docs)) | ||||||
| return types.SimpleNamespace(config=config, env=env) # pyright: ignore[reportReturnType] | ||||||
|
|
||||||
| def test_automatic_redirects_extends_dict_in_final_pass(self): | ||||||
| app = self.fake_app(rediraffe_redirects={}, found_docs={'foo/a-b', 'index'}) | ||||||
| automatic_redirects._automatic_redirects(app, app.env) | ||||||
| assert app.config.rediraffe_redirects == {'foo/a_b': 'foo/a-b'} | ||||||
|
|
||||||
| def test_automatic_redirects_skips_per_package_pass(self): | ||||||
| app = self.fake_app( | ||||||
| package='pathops', rediraffe_redirects={}, found_docs={'foo/a-b', 'index'} | ||||||
| ) | ||||||
| automatic_redirects._automatic_redirects(app, app.env) | ||||||
| assert app.config.rediraffe_redirects == {} | ||||||
|
|
||||||
| def test_automatic_redirects_preserves_existing_user_redirects(self): | ||||||
| app = self.fake_app( | ||||||
| rediraffe_redirects={'old/page': 'new/page'}, found_docs={'foo/a-b', 'index'} | ||||||
| ) | ||||||
| automatic_redirects._automatic_redirects(app, app.env) | ||||||
| assert app.config.rediraffe_redirects == {'old/page': 'new/page', 'foo/a_b': 'foo/a-b'} | ||||||
|
|
||||||
| @pytest.mark.parametrize('value', ('redirects.txt', None)) | ||||||
| def test_automatic_redirects_raises_when_rediraffe_redirects_not_a_dict( | ||||||
| self, value: str | None | ||||||
| ): | ||||||
| # The extension requires rediraffe_redirects to be a dict; a filename | ||||||
| # (str) or None is a misconfiguration and should fail loudly. | ||||||
| app = self.fake_app(rediraffe_redirects=value, found_docs={}) | ||||||
| with pytest.raises(AssertionError, match='be a dict'): | ||||||
| automatic_redirects._automatic_redirects(app, app.env) | ||||||
|
|
||||||
| def test_alias_clobbering_user_redirect_raises(self): | ||||||
| app = self.fake_app( | ||||||
| rediraffe_redirects={'how_to/manage_libraries': 'custom/target'}, | ||||||
| found_docs={'how-to/manage-libraries', 'index'}, | ||||||
| ) | ||||||
| with pytest.raises(ValueError, match='clobbered'): | ||||||
| automatic_redirects._automatic_redirects(app, app.env) | ||||||
|
|
||||||
| def test_alias_chaining_with_user_redirect_raises(self): | ||||||
| app = self.fake_app( | ||||||
| rediraffe_redirects={'how-to/manage-libraries': 'custom/target'}, | ||||||
| found_docs={'how-to/manage-libraries', 'index'}, | ||||||
| ) | ||||||
| with pytest.raises(ValueError, match='chain with manually'): | ||||||
| automatic_redirects._automatic_redirects(app, app.env) | ||||||
|
|
||||||
| def test_user_redirect_chaining_with_alias_raises(self): | ||||||
| app = self.fake_app( | ||||||
| rediraffe_redirects={'custom/alias': 'how_to/manage_libraries'}, | ||||||
| found_docs={'how-to/manage-libraries', 'index'}, | ||||||
| ) | ||||||
| with pytest.raises(ValueError, match='should point to'): | ||||||
| automatic_redirects._automatic_redirects(app, app.env) | ||||||
|
|
||||||
| def test_user_redirect_to_canonical_name_ok(self): | ||||||
| app = self.fake_app( | ||||||
| rediraffe_redirects={'custom/alias': 'how-to/manage-libraries'}, | ||||||
| found_docs={'how-to/manage-libraries', 'index'}, | ||||||
| ) | ||||||
| automatic_redirects._automatic_redirects(app, app.env) | ||||||
| assert 'custom/alias' in app.config.rediraffe_redirects | ||||||
| assert len(app.config.rediraffe_redirects) > 1 | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.