Promote tests and fix regression - #276
Conversation
Note that there is a regression: a `children:` attribute has erased an attribute defined in place
This adds also a small difference in behavior in terms of reported errors: a duplicated attribute will be ignored and so not be reported if non-standard. This is fine as anyway the attribute eraser will be reported. The regression was introduced by 326f479, and before it was working but mostly by chance.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe change adds Merge Risk: 🟡 Moderate · up to The PR promotes regression tests and changes attribute-merging behavior, but the current head still contains a compile-blocking renderer issue, so it should not merge until that mismatch is fixed. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (6 skipped: 6 unsupported.) Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a061526-5064-459f-8195-f47313b93e79
📒 Files selected for processing (9)
src/compiler/ast.mlsrc/compiler/compile.mlsrc/compiler/frontmatter.mlsrc/compiler/renderers.mltest/compiler/dash-separator/children-slides.t/run.ttest/compiler/slides.t/run.tvendor/github.com/panglesd/cmarkit/src/cmarkit.mlvendor/github.com/panglesd/cmarkit/src/cmarkit.mlivendor/github.com/panglesd/cmarkit/src/cmarkit_html.ml
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
| attrs | ||
| |> add_escaped_attrs ~keep_base:false "data" l | ||
| |> add_escaped_attrs ~keep_base:false "type" "image/svg+xml" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Accept and forward ~keep_base through add_escaped_attrs. Cmarkit.Attributes.add requires this label, but the current renderer path does not provide it. Update the helper and its call sites so the argument is accepted and forwarded consistently; otherwise the current head fails to compile.
📍 Affects 2 files
src/compiler/renderers.ml#L215-L217(this comment)vendor/github.com/panglesd/cmarkit/src/cmarkit.mli#L316-L316
1 participant
#270 did not promote tests, and in particular 326f479 introduced a regression.
This PR promotes the tests first, then fix the regression. Which was that
children:attributes erased attributes associated to the children.No changelog since the regression was not released.