Conversation
d9a392d to
ac17b63
Compare
|
If this is in a stable place, I'm fine to merge this now and sort out the issues in followup PRs. |
|
It implements some basic cases and doesn't seem to break anything at least :-) |
|
I am working now on finishing this. |
|
Weird, tests were passing before, fixing. |
|
Sigh, I asked the LLM to fix ameba warnings and it choose wrong on every one of them. Fixed now :-) |
|
Wonder if the Edit: Same with CLAUDE.md and the like |
Deleted and ignored. |
|
I can't spend any more time working on this PR. |
|
I can carry it from here if you want. |
That would be great, thanks. |
|
I may have some time to work on markpdf these days if the project is still alive. Is there any problem with this PR? |
|
Guess it's dead. If someone wants to pick up the footnotes implementation the code is there. |
|
Sorry about the tantrum. Costs me nothing to keep this PR open for whenever someone wants it. |
There was a problem hiding this comment.
Pull request overview
Implements GitHub Flavored Markdown (GFM) footnotes end-to-end in Markd: parsing inline footnote references and block footnote definitions, post-processing to normalize numbering / resolve nested references, and HTML rendering including backrefs.
Changes:
- Added a new block rule and node types to recognize and store footnote definitions and inline footnote references.
- Added a post-parse footnote processing pass to resolve references (including nesting), normalize numbering, and move definitions to the end.
- Implemented HTML output for footnote refs/definitions and enabled fixture-based coverage for the new behavior.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/markd/rules/footnote_definition.cr | New block rule to match and collect footnote definition blocks. |
| src/markd/rule.cr | Adds footnote definition start regex and treats [ as “maybe special”. |
| src/markd/renderers/html_renderer.cr | Renders footnote refs/definitions and backrefs; supports valueless (boolean) attributes. |
| src/markd/renderer.cr | Adds abstract renderer hooks for footnote nodes (but currently has a duplicate abstract method). |
| src/markd/parsers/inline.cr | Parses [^label] as a new inline Footnote node in GFM mode. |
| src/markd/parsers/block.cr | Adds passes to parse definition contents as blocks and to resolve/number/move footnotes. |
| src/markd/node.cr | Introduces Footnote / FootnoteDefinition node types and container classification changes. |
| spec/fixtures/gfm-regression.txt | Enables existing regression fixtures that exercise footnotes (including nesting). |
| spec/fixtures/gfm-extensions.txt | Enables GFM extension fixtures for footnotes and escaping behavior. |
| .vscode/launch.json | Removes editor launch configuration from the repo. |
| .gitignore | Ignores spec binary and editor/agent files; broadens VS Code ignore to entire folder. |
Suppressed comments (1)
src/markd/parsers/block.cr:156
footnote_definitionsis deleted from insidefootnote_definitions.each, which can raise due to Hash mutation during iteration. Iterate overkeyssnapshot instead.
footnote_definitions.delete footnote_title
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
I'll do a first round of review using Copilot first. @nobodywasishere hasn't been active in this project for a while—I wonder if she's been busy since joining the Crystal core team. |
|
I am still here. Over the past year though have been dealing with a lot of personal stuff, and wrestling with how programming is changing in the advent of AI. Hard to find the joy in doing things if there's a machine that'll do it much faster than you. |
Understand the feeling |
|
Tried to accept the fix suggestions but after the 1st one now they are stale :-D I will get a minion to look at them |
Cover the edge cases from the Copilot review on PR icyleaf#78 that had no spec coverage: - [^a](url) keeps the footnote ref and the parentheses as text instead of consuming (url) as a link destination - a mid-line [^x]: does not start a footnote definition - nested footnote references resolve with arbitrary label characters
Co-authored-by: Margret Riegert <margret@eowyn.net>
Co-authored-by: Margret Riegert <margret@eowyn.net>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
- renderer.cr: remove duplicate thematic_break abstract method declaration - rule.cr: anchor FOOTNOTE_DEFINITION_START to line start so mid-line [^x]: does not incorrectly start a block-level definition - block.cr: iterate over a snapshot of hash keys instead of mutating footnotes/footnote_definitions during each-iteration (fixes potential Hash modified during iteration errors) - block.cr: broaden nested-footnote label regex from [\w-]+ to [^\]]+ so nested references with arbitrary label characters are detected - inline.cr: skip inline-link (...) parsing for footnote openers so [^a](url) renders as a footnote ref with (url) preserved as text
Cover the edge cases from the Copilot review on PR icyleaf#78 that had no spec coverage: - [^a](url) keeps the footnote ref and the parentheses as text instead of consuming (url) as a link destination - a mid-line [^x]: does not start a footnote definition - nested footnote references resolve with arbitrary label characters
- add spaces around macro expression in Node#to_s - drop redundant return in Inline#match
# Conflicts: # shard.yml
|
Comments addressed, updated from main |
The tartrazine require test compile-runs spec/fixtures/tartrazine.cr, which needs the tartrazine dev dependency, so --without-development broke spec compilation on every OS in the matrix (same fix the ameba job already got). Also fix ameba warnings in the fixture itself.
v1.6.4 no longer compiles with current Crystal (undefined method 'next_string_array_token' on Crystal::Lexer). v1.7.0 builds with Crystal >= 1.19 and passes this repo's .ameba.yml with no changes.
The inline parser tracks positions as byte offsets, but the footnote label was cut from the text with character-based slicing. Any multi-byte character earlier in the line (an emoji, an accented letter) shifted the label: the reference matched no definition and degraded to literal text like '✅[^]'. Cut the label with byte_slice instead, and guard it with a regression fixture covering emoji and accented characters in paragraphs and table cells.
This is, AFAIK a full implementation of footnotes as per the specs we have on the repo.