Fix topmatter clobbering validated config values, and enforce global_only - #1197
Open
dchaudhari7177 wants to merge 1 commit into
Open
Conversation
…only
Two bugs in `merge_file_level`, both reachable from ordinary front matter.
**Validated values were overwritten.** The loop called
`validate_field(new, field, value)` and then `setattr(new, name, value)`
with the *raw* value. Validators that normalise their input write the
result back on the instance, so that write was immediately undone:
---
myst:
fence_as_directive: ["mermaid"]
---
left a `list` in a field the rest of the code treats as a `set`.
Fixed by assigning before validating and letting the validator have the
last word -- the order `__post_init__` already uses, where the value is on
the instance before `validate_fields` runs. An invalid value now has to be
rolled back explicitly, so a rejected value is not reported *and* kept.
**`global_only` was declared and never checked.** Five fields carry the
metadata -- `heading_slug_func`, `update_mathjax`, `mathjax_classes`,
`suppress_warnings`, `inventories` -- and topmatter could set all of them.
---
myst:
heading_slug_func: github
---
put the preset *name* in the field instead of the function it names, so
every heading raised `'str' object is not callable [myst.heading_slug]` and
no slugs were generated. Topmatter now warns and keeps the global value.
The documented contract was already right: `_docs.py` filters `global_only`
fields out of the `:scope: local` table, so `configuration.md` has never
listed these five as front-matter settable. Only the enforcement was
missing.
Seventeen tests, eight of which fail on master. Beyond the two bugs they
pin the surrounding behaviour the reordering runs through: `merge_topmatter`
still merges rather than replaces, a rejected field falls back to the
*global* value rather than the dataclass default, one bad key does not
abandon the rest of the topmatter, and the global config is not mutated.
tests/: 1252 passed, 18 skipped. The 3 failures in my run
(`test_cmdline[40-linkify]`, `test_extended_syntaxes`,
`test_extended_syntaxes_text`) are identical on master with this branch
stashed. ruff 0.15.20 clean.
Fixes executablebooks#1167
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.
Fixes #1167. Two bugs in
merge_file_level, both reachable from ordinary front matter.1. Validated values were overwritten
The loop called
validate_field(new, field, value)and thensetattr(new, name, value)with the raw value. Validators that normalise their input write the result back on the instance, so that write was immediately undone.The issue reports this via
heading_slug_func, but it is not specific to that field:check_fence_as_directiveconverts the sequence to asetand assigns it; the raw list then landed on top. The rest of the code does set operations on that field, so this is a wrong type rather than merely a different one.Fixed by assigning before validating and letting the validator have the last word — which is the order
__post_init__already uses, where the value is on the instance beforevalidate_fieldsruns. The consequence is that an invalid value now has to be rolled back explicitly, so a rejected value isn't reported and kept; there are two tests for that, including one asserting it rolls back to the global value rather than the dataclass default.2.
global_onlywas declared and never checkedFive fields carry the metadata —
heading_slug_func,update_mathjax,mathjax_classes,suppress_warnings,inventories— and topmatter could set all of them.put the preset name in the field instead of the function it names, so every heading raised
'str' object is not callable [myst.heading_slug]and no slugs were generated. Topmatter now warns and keeps the global value.Worth noting: the documented contract was already correct.
_docs.py:137filtersglobal_onlyfields out of the:scope: localtable, soconfiguration.mdhas never listed these five as front-matter settable. Only the enforcement was missing — which is why I made topmatter reject them rather than making the validation stick.Tests
Seventeen new tests in
tests/test_config_topmatter.py. Eight fail onmaster:The other nine are guards for the paths the reordering runs through, and are the ones I'd want if someone later re-simplified this:
merge_topmatterstill merges rather than replaces, the file-level value still wins on a merged field, one bad key doesn't abandon the rest of the topmatter, an unknown field is still reported, and the global config isn't mutated.Three failures in my local run —
test_cmdline[40-linkify],test_extended_syntaxes,test_extended_syntaxes_text— are identical onmasterwith this branch stashed, so they aren't from this change.No
CHANGELOG.mdentry: entries there carry<gh-pr:NNNN>links and appear to be written at release time. Happy to add one if you'd rather they land with the PR.