🐛 FIX: attrs: KeyError on aliased image attributes, duplicate {#id} headings, new-tab link attributes - #1200
Open
chrisjsewell wants to merge 6 commits into
Open
🐛 FIX: attrs: KeyError on aliased image attributes, duplicate {#id} headings, new-tab link attributes#1200chrisjsewell wants to merge 6 commits into
chrisjsewell wants to merge 6 commits into
Conversation
`copy_attributes` rebinds `key` to the aliased (node) name before the
value is validated, but then interpolated `token.attrs[key]` into the
warning message. For a short form such as `{w=1x}` that key
does not exist on the token, so an invalid value raised
`KeyError: 'width'` and aborted the build instead of warning.
Interpolate the value already in hand; `token.attrs[key]` was only ever
a re-fetch of it. The message text is unchanged for the long forms, and
an alias now produces the same message as its long form.
`generate_heading_target` appended the title-derived name to the node and
called `note_implicit_target`, which re-registers every name already on
the node -- including the explicit `{#id}` name that `copy_attributes`
had just added and registered. The node therefore collided with itself:
docutils demoted the explicit name into `dupnames` while its name map
still pointed at the node, so a second heading carrying the same `{#id}`
raised `ValueError: list.remove(x): x not in list` and no page was
written. The same happened for headings rendered as rubrics inside a
directive.
Register only the newly derived implicit name, and restore the explicit
names afterwards. Duplicates now produce docutils' own
`Duplicate explicit target name` warning, exactly as duplicate ids on
paragraphs already did, and a single `{#id}` heading keeps that name
instead of losing it to `dupnames`. docutils 0.23 guards against the
self-collision on its own, so this only affects earlier versions --
which is every version a released Sphinx can be paired with.
`myst_links_external_new_tab` emitted `rel="noreferer noopener"`, with one `r`. No browser recognises that token, so the referrer was sent anyway and the option only ever delivered half of what it promised. Spell it `noreferrer`. The single fixture that pinned the misspelling is updated with it.
`render_link_url` wrote the config-driven `target` and `rel` onto the
token before `copy_attributes` read the attribute group, so an authored
`{target=_self rel=nofollow}` was silently replaced by
`target="_blank" rel="noreferrer noopener"`.
Apply the config values only for keys the author did not supply. The two
keys are independent, so authoring just one of them leaves the other on
its configured value. This is non-breaking for anyone not authoring
`target` or `rel` on an external link: with no attribute group the
rendering is unchanged.
The option's help text, which is the whole of its user-facing
documentation, now says which attributes it sets and that authored
values win.
An out-of-range `emphasize-lines` reported `emphasize_lines: out of range(1-3` -- the closing parenthesis was missing from the format string. Sphinx's own equivalent message, `line number spec is out of range(1-%d): %r`, is unbalanced too; this makes MyST's copy read correctly rather than matching it.
Review follow-ups on the preceding five commits, tests only:
- `test_help_text` now asserts the `myst_links_external_new_tab` help
string mentions `rel="noreferrer noopener"`, so reverting it is no
longer invisible to the suite.
- A `myst-config.txt` row pins that when an alias and its long form are
both given (`{w=1x width=2x}`), each warning reports its own value;
before the alias fix the alias's warning reported the long form's value.
- The `heading with id` doctree row uses an H1 so it no longer carries the
unrelated "headings start at H2" message; the load-bearing section line
is unchanged.
- `test_parse_linenos.py` skips at module level when Sphinx is missing,
since the helper it tests imports `sphinx.util` at call time.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
[i](f.png){w=1x}(and{h=…},{a=…}) aborted the build withKeyError: 'width'; an invalid valuegiven by its short alias now warns, exactly as the long form already did.
{#id}aborted the build withValueError: list.remove(x): x not in listand wrote no page; they now emit docutils' ordinaryDuplicate explicit target namewarning, as duplicate ids on paragraphs already did. A single{#id}heading also stops silently losing that name.
myst_links_external_new_tabemittedrel="noreferer noopener"— oner, a token no browserrecognises. It is now spelled
noreferrer.myst_links_external_new_taboverwrote an authored{target=…}/{rel=…}; the config values now applyonly to keys the author did not supply.
emphasize_linesrange warning readout of range(1-3— the closing parenthesis is restored.Compatibility
{w=1x}/{h=2y}/{a=middle}previously abortedsphinx-build(exit 2, no HTML) and the docutils CLI (exit 1). The long forms' message text isbyte-unchanged — the new fixture row's expected output is a byte copy of the existing long-form row's.
One further, deliberate change: where an alias and its long form are both given for the same
attribute (
{w=1x width=2x}), the alias's warning now reports the alias's own value ('1x') instead ofthe long form's (
'2x', which is what it happened to print before). Both warnings are still emitted, andthe long form's own message is unchanged. A fixture row pins it.
{#id}headings: crash → warning. The build now completes and writes the page. The warningis docutils' own message under Sphinx's
[docutils]subtype — character for character what the paragraphcase already produced — so it is suppressible today with
suppress_warnings = ["docutils"]. HTML ids areunchanged (measured: identical id sets with and without the fix, on docutils 0.21.2 and 0.22.4). A single
{#id}heading now keeps that name innamesinstead of losing it todupnames. The change is ameasured no-op on docutils 0.23, which guards against the underlying self-collision itself.
noreferer→noreferrer. The old value was inert, so nothing that worked stops working. Nothing inthe docs mentioned either spelling.
target/relnow win. Non-breaking for anyone not authoring them: with no attribute groupthe rendering is byte-identical. The two keys are independent. An authored empty value counts as
supplied, so
[a](url){rel=""}now rendersrel=""where it previously rendered the configured value;an unquoted
{rel=}does not parse as an attribute group at all, before or after. The option's helptext — its only user-facing documentation — now states what it sets and that authored values win.
out of range(1-3). Warning text only. Note this now differs by one character from Sphinx's ownline number spec is out of range(1-%d): %r, which is unbalanced too; the divergence is deliberate.noreferer→noreferrerintests/test_renderers/fixtures/myst-config.txt.Tests
myst-config.txtrow[attrs_inline_image_warnings_aliases](
[a](b){w=1x h=2x a=other }). Before: the row raisedKeyError: 'width'insidecopy_attributes.After: three
Invalid '…' attribute valuewarnings, output byte-identical to the long-form sibling row.A second row
[attrs_inline_image_warnings_alias_and_long]([a](b){w=1x width=2x}) pins thealias-plus-long-form case: before, both warnings read
'2x'; after,'1x'then'2x'. Both rows runSphinx-less, so the docutils-only CI job covers them too (green on docutils 0.20 and 0.23).
{#id}headings — new sourcedirtests/test_sphinx/sourcedirs/attrs_block_duplicate_ids/with three duplicate pairs (headings, paragraphs, and headings inside a
{note}fence, i.e. the rubricpath) plus
test_attrs_block_duplicate_ids. Before:ValueError: list.remove(x): x not in list, no page.After:
build succeeded, exactly threeDuplicate explicit target namewarnings,index.htmlwritten.Two
attributes.mdrows pin the doctree: before,<section dupnames="hid" ids="hid" names="first">plusan INFO
Duplicate implicit target name: "hid"., and<rubric dupnames="rid" … names="first">; after,<section ids="hid" names="hid first">and<rubric ids="rid" level="2" names="rid first">.Note: the integration test passes without the fix on docutils 0.23, which already guards the
self-collision. Every Sphinx-paired CI cell runs docutils < 0.23 (Sphinx 8 caps
<0.22, Sphinx 9 caps<0.23), so the test is genuinely red in CI on the unfixed tree.noreferrer— the existing[links-external-new-tab]row was updated to the correct spelling first;it then failed with
-noreferrer / +noreferer, and passes after the source change.target/rel— new row[links-external-new-tab-authored]covering both keys authored,each key alone, and an autolink. Before: all four rendered
rel="noreferrer noopener" target="_blank".After:
rel="nofollow" target="_self",rel="noreferrer noopener" target="_self",rel="nofollow" target="_blank",rel="nofollow" target="_blank". The option's help text is pinned by anew assertion in
tests/test_docutils.py::test_help_text(red with the old text, green with the new).out of range(1-3)— newtests/test_renderers/test_parse_linenos.py, a unit test on the_parse_linenosstatic method (the message is unreachable without Sphinx, so a fixture row cannot pinit; the module skips itself when Sphinx is absent). Before: the regex did not match
out of range(1-3.After: it matches, and a valid case still returns
[2].Gates
Python 3.11, four environments, all measured at the branch tip:
tests/test_renderers398 passed 8 skipped;tests/test_sphinx24 passed; the rest 831 passed 3 skipped. 1253 passed, 0 failed (master baseline 1245).
1249 passed, 0 failed (master baseline 1241).
check-myst-docutilsjob's four files): 180 passed, 0 failed(baseline 177).
pre-commit run --all-files: all hooks passed, exit 0.Every delta is the new tests; the skip lists are unchanged from master in all four environments.
Changelog lines
KeyErroron an invalid image attribute given by its alias ({w=…},{h=…},{a=…}){#id}on headings warns instead of aborting the build, and a single{#id}headingkeeps its name
myst_links_external_new_tabspellsrel="noreferrer"myst_links_external_new_tabno longer overwrites an authoredtargetorrelemphasize_linesout-of-range warningFollow-ups noticed
render_link_urlmutates the parser's own token stream (token.attrs) just to pass two values tocopy_attributesthree lines later — the only writes totoken.attrsin the package. Passing thedefaults to
copy_attributes, or setting them on the reference node when absent, would be cleaner; thatis a refactor, not a bug fix.
warning's text baked into
names(measured on the unfixed tree with a non-crashing trigger:names="…\ (warning/2)\ duplicate\ explicit\ target\ name:\ "dupe".h"; absent on 0.22.4). Pre-existingand untouched here; computing the rubric's implicit text before noting the explicit target would fix it.
line number spec is out of range(1-%d): %r(sphinx/directives/code.py:131,297,483in8.2.3 and 9.0.4) is unbalanced in the same way.
tests/test_renderers/test_fixtures_docutils.py::test_docutils_roles[112-code]fails iftests/test_sphinxis collected beforetests/test_renderersin one process (Sphinx'scoderole leaksa
language=""default into the global docutils role registry). Pre-existing; never fires in CI or tox,which collect alphabetically.
docs/syntax/optional.md) is blocked on an mdit-py-pluginsrelease carrying the span/link class-accumulation fix:
pyproject.tomlpinsmdit-py-plugins~=0.6,>=0.6.1, and 0.6.1 is the newest release on PyPI, so "classes accumulate" is notyet true for spans and links with the dependency as pinned.
CHANGELOG.mdis deliberately untouched; the entries above are for the maintainer.Out of scope by ruling
input.
_attr_block_rule({.a}{#b}on one line applies only thefirst group); re-enabling it turns such lines into paragraphs.
{-=v},{1=v}) are accepted.[a [b]{.c} d](u)) breaks the link.ParseErrorpositions as warnings — a feature, not a fix.attrs_blockbefore a{directive}fence, or before an<img>underhtml_image, being dropped —feature-sized (the directive mapping exists for
myst_fence_as_directiveand could be reused).