Repository navigation
Recognize block-style heading attributes - #2250
Conversation
katrinafyi
left a comment
katrinafyi
left a comment
There was a problem hiding this comment.
Hi :) I have a drive-by comment.
It would be nice to avoid the need to copy all of the paragraph text into a String, since it could be quite large. AFAICT, copying would only be needed if the parser returned the heading attribute across different Text events, which I think it would only do if it was split up by another tag (like bold, italic, etc). That shouldn't be the case here.
Separately, is there a spec for this markdown extension? I would also be curious about the handling of these edge cases and whether they act as expected according to the spec (if there is one). That said, some of these are quite mean and it's probably fine to overapproximate the fragment identifiers so maybe not a big deal.
{ #spaces-around }
# a
a {#text-before}
# a
{#text-after} a
# a
a
{#line-before}
# a
{#line-between}
# a
```
{#inside-code-block}
```
# a
`{#inside-code-inline}`
# a
*{#inside-bold}*
# a
> {#inside-quote}
> # a
{ #multiple-attributes } { #multiple-attributes2 }
# a
{} #between-empty-sets {}
# a
{{ #double-braced }}
# a
{{#double-braced-no-space}}
# a
{
#across-lines
}
# a| let mut in_paragraph = false; | ||
| let mut heading_text = String::new(); | ||
| let mut heading_id: Option<CowStr<'_>> = None; | ||
| let mut heading_block_id: Option<String> = None; |
|
Thanks for the review. I pushed a follow-up that:
Checks run locally:
|
|
@sjh9714, going forward, please make sure all comments are cleaned up before posting. We prefer human commentary here, even if imperfect. It's more productive to hear your own thoughts so we can help you best. |
2ae134e to
6058863
Compare
|
Got it, thanks for pointing this out. I’ll take more care with my comments going forward and make sure they’re in my own words before posting. |
Parse supported preceding attributes for ATX and Setext headings while rejecting malformed, nested, multiline, and non-heading attribute groups.
Add focused tests for grouped, quoted, and escaped attributes.
|
@katrinafyi, the paragraph String buffer is now gone. :) Instead, we inspect borrowed source slices only, so we only allocate the extracted ID. I also checked your examples against MyST’s attrs_block implementation in mdit-py-plugins and added unit and integration coverage. We now match its handling of multiple groups on one line: I also added tests for quoted attribute values, so things like Overall, I'm still not 100% happy with that code. We made attribute parsing much harder to understand, but that's mainly because attribute parsing is hard. In the future, I'd like us to refactor that code. It's okay for now, though, I'd say. |
|
The failing test is unrelated. See #2294 |
|
@sjh9714, thanks for the initial work on this. |
3 participants
Summary
Fixes #2249.
--include-fragmentsalready recognizes heading IDs written inline as### Heading {#id}. This adds the same fragment recognition for MyST-style preceding block attributes such as:{#block-id} ### HeadingThe change is scoped to Markdown fragment extraction. A block attribute is only applied to the immediately following heading, and the normal generated heading fragment is still emitted.
Changes
{#id}paragraph before a heading during Markdown fragment extraction.Testing
PATH="$HOME/.cargo/bin:$PATH" cargo test -p lychee-lib test_extract_fragments_from_preceding_heading_attributes -- --nocapture(failed before the fix, passed after)PATH="$HOME/.cargo/bin:$PATH" cargo test -p lychee-lib extract::markdown -- --nocapturePATH="$HOME/.cargo/bin:$PATH" cargo fmt --all -- --checkPATH="$HOME/.cargo/bin:$PATH" cargo clippy -p lychee-lib --all-targets --all-features -- -D warningsPATH="$HOME/.cargo/bin:$PATH" cargo test -p lychee-libgit diff --checkNote: I used Codex while preparing this change, reviewed the final diff, and ran the listed checks locally.