fix(MD007): respect explicit config for nested lists under ordered parents while preserving DWIM consistency#711
Conversation
…rents while preserving DWIM consistency
When `indent` is explicitly configured (e.g. `indent = 4`), we disable
the default exemption for unordered lists under ordered parents to
enforce the user's configuration.
However, the previous "Do What I Mean" (DWIM) logic (which automatically
switches to Fixed style when `indent` is explicit but `style` is not)
was buggy for deeper nesting levels in mixed lists:
```md
1. Ordered Parent
- Level 1 Unordered (3 spaces - text-aligned, allowed by DWIM)
- Level 2 Unordered (5 spaces - text-aligned)
```
For Level 2, because its immediate parent is unordered, the previous
DWIM logic unconditionally forced `Fixed` style (4 spaces), resulting in
an inconsistent layout where Level 2 was only indented 1 space relative
to Level 1.
We refine the DWIM logic to preserve consistency:
- If the list has an ordered ancestor (we are in a mixed list chain), we
check if the parent is offset. If it is (because it chose
TextAligned), we propagate the TextAligned style to the child to
maintain consistency.
- If the list is pure unordered (no ordered ancestor), we continue to
force Fixed style as DWIM intended, preventing collisions when fixing
misindented items.
Assisted-by: Antigravity with Gemini
rvben
left a comment
There was a problem hiding this comment.
Thanks for digging into this, and for the clear write-up of the DWIM reasoning. I think the diagnosis of the level-2 inconsistency is real, but I have to push back on the fix, because the !is_explicit gate reintroduces #638 and makes rumdl fmt corrupt the document.
Repro. Issue #638's reporter config was exactly indent = 2, style = "fixed", on exactly this snippet. With --no-cache on both branches:
main: Success: No issues found
pr711: doc.md:4:1: [MD007] Expected 2 spaces for indent depth 1, found 3 [*]
doc.md:5:1: [MD007] Expected 4 spaces for indent depth 2, found 5 [*]
rumdl fmt then rewrites to 2/4 spaces. Rendered through cmark (the CommonMark reference implementation):
<!-- before --> <!-- after fmt, with this PR -->
<ol> <ol>
<li>Some text <li>Some text</li>
<ul> </ol>
<li>Indented text <ul>
... <li>Indented textThe bullet list detaches from the ordered item and becomes a sibling top-level list. #638's reporter cited that exact output in their issue under "Example of list not correctly rendered." I reproduced the same detachment with 10. Some text / - Indented, where a correctly-nested 4-space child gets "fixed" down to 2.
The root cause is that the exemption is not a default that explicit config should override. At indent = 2, strict fixed indentation under 1. is below the ordered item's content column, so it is not merely a style choice, it is unrenderable. And style = "fixed" is documented as the markdownlint-compatible mode (md007_config.rs:13), while markdownlint does not apply MD007 to sublists of ordered lists at all (as the comment at md007_ul_indent.rs:423 notes). So enforcing fixed indentation there moves us away from the behavior that setting exists to provide.
Relatedly, test_issue_638_unordered_under_ordered_style_fixed is changed here from assert!(result.is_empty(), ...) to asserting the two warnings and the broken fix() output. That is why the suite stays green. It would be better to leave that test asserting the correct behavior and let it fail.
What I'd suggest: drop !is_explicit from the gate at md007_ul_indent.rs:453 and restore the #638 test. That unblocks the correctness problem on its own.
I'd like to keep the calculate_expected_indent refinement, but I couldn't reproduce the inconsistency it targets on main. When a bullet is genuinely nested under an ordered ancestor, the exemption fires and calculate_expected_indent is never reached, so the "level 2 forced to Fixed" path only becomes reachable once !is_explicit disables the exemption. The one path where the DWIM arm is reachable on main (chain_ok false, e.g. 100. Ordered / - L1 / - L2) changes the message from Expected 8 to Expected 7, but I diffed the emitted fmt output on both branches and it is byte-identical.
Could you share the document and config that trigger the inconsistency on main? If there's a real case, that change can land on its own with a test that fails without it.
Two non-blocking notes on the refinement, for whenever it lands:
- The new parameter is named
has_ordered_ancestorbut receivesthreshold_ok(line 483), which means "an ordered ancestor has its content column at or left of this bullet's actual marker column." A bullet that does have an ordered ancestor but sits too far left getsfalse. Something likenested_under_orderedwould read more honestly. - The new arm (lines 184-196) is behaviorally identical to the smart-default arm at 211-227, including the
saturating_sub(2)marker-width assumption. Worth extracting into one helper.
|
Will dig into the suggested direction, but just to provide the context of where this is coming up: We've adopted We have a lot of markdown -- a huge, living design document tree for the entire programming language, and proposals for every change to it. And then more for the design documents of the toolchain and for skill files. It keeps finding ... challenging cases for formatting. The specific case here comes from some markdown in skills files that roughly looks like: 1. Some ordered step of a process
- An incorrectly over-indented (by one space) sub-listAnd more nesting beyond that. The concrete file is: https://github.com/carbon-language/carbon-lang/blob/trunk/.agents/skills/builtins/SKILL.md our goal is to have a strong normalizing markdown formatter that reliably canonicalizes the line wrapping, the indentation, etc. Our config is: https://github.com/carbon-language/carbon-lang/blob/trunk/.rumdl.toml |
|
Thanks @chandlerc for the feedback and context. That helps. I might not be available so much coming week, so it's expected if I not respond as fast as normal. :) |
rvben
left a comment
There was a problem hiding this comment.
Thanks @chandlerc, and sorry to come back with a blocker on this one.
The indent_explicit half is good and I want to merge it. The problem is style_explicit, which I think has to come out.
is_explicit folds in style_explicit, so setting style = "fixed" switches off the ordered-parent exemption. calculate_expected_indent's Fixed branch then returns nesting_level * indent with no floor. A bullet nested under an ordered parent can't be indented less than that parent's content column without leaving the item, so the fix walks it straight out of the list.
That's what forced test_issue_638_unordered_under_ordered_style_fixed to flip from "must not be flagged" to "flag both and fix". The output it now asserts is the document that the reporter of #638 filed as broken. From the issue, under the heading "Example of list not correctly rendered":
1. Some text
- Text not rendered as indented
- more indentedThe PR asserts fix() produces "# Title\n\n1. Some text\n - Indented text\n - more indented\n". Both through cmark:
<!-- input: 3 and 5 spaces -->
<ol>
<li>Some text
<ul>
<li>Indented text
<!-- what fix() now produces: 2 and 4 spaces -->
<ol>
<li>Some text</li>
</ol>
<ul>
<li>Indented textThe bullet list comes out as a sibling of the ordered list rather than a child of it. Same thing through the CLI: with indent = 2, style = "fixed" this branch rewrites the file and reports "Fixed 2/2 issues", where main finds nothing to fix. So rumdl fmt would quietly restructure people's documents, and #638 is back.
Worth saying plainly, because it's the part that unsettles me: CI is green here only because the PR rewrote the test that would have caught it. I don't think that was deliberate, and the new expectation looks perfectly reasonable if you aren't holding the issue thread in your head. But it's why I want the guard back before any of this lands.
So, concretely: drop style_explicit from is_explicit and restore the test_issue_638_* expectation. Keep everything else. The indent_explicit propagation and test_unordered_under_ordered_explicit_config_flagged_and_fixed look right to me, and I checked that case is safe: 1. Parent item has content column 4, the bullet moves from 5 to 4, and cmark still nests it. That subset can go in as soon as you push it.
Honoring an explicit style = "fixed" under ordered parents is still a fair thing to want, and I don't want to just wave it off. The way to do it without breaking nesting is to clamp rather than exempt: expect max(nesting_level * indent, parent_content_col). It's a bigger change than it first looks, because it starts flagging documents that are silent today (a bullet at 4 spaces under 1. would newly be rewritten to 3), and it has to account for also_acceptable, blockquote depth, and ol-align-column. I'll pick that up separately so it gets its own tests, with the #638 test sitting there as the guard while I do.
Two smaller things:
md007_ul_indent.rs:483 passes threshold_ok into a parameter named has_ordered_ancestor. They're close but not the same. threshold_ok means "some ordered ancestor at this blockquote depth has its content column at or left of this marker", and it deliberately leaves out chain_ok. Either rename the parameter, or pass threshold_ok && chain_ok if the chain is what you meant.
The example in the PR description doesn't actually nest, which may be what sent the level-2 logic astray:
1. Ordered Parent
- Level 1 Unordered
- Level 2 Unordered1. has content column 4, so a bullet at 3 spaces is already outside the ordered item. cmark gives <ol><li>Ordered Parent</li></ol><ul>..., with Level 1 as a sibling. If there's a real inconsistency at level 2, could you post a repro where the chain genuinely nests? I'd rather fix that one properly than lose track of it.
|
The clamp I promised above shipped in v0.2.30 (1f3a32d). With an explicit Concretely for your SKILL.md case: with |
|
So, v0.2.30 definitely improves this, along with using an explicit style="fixed" I'm not seeing any more issues like this. Sending PRs for the next issues I've found. =D |
When
indentis explicitly configured (e.g.indent = 4), we disable the default exemption for unordered lists under ordered parents to enforce the user's configuration.However, the previous "Do What I Mean" (DWIM) logic (which automatically switches to Fixed style when
indentis explicit butstyleis not) was buggy for deeper nesting levels in mixed lists:For Level 2, because its immediate parent is unordered, the previous DWIM logic unconditionally forced
Fixedstyle (4 spaces), resulting in an inconsistent layout where Level 2 was only indented 1 space relative to Level 1.We refine the DWIM logic to preserve consistency:
Assisted-by: Antigravity with Gemini