-
Notifications
You must be signed in to change notification settings - Fork 40
feat: make release notes insert parts from markdown #599
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This isn't right yet; it inserts consecutive parts more deeply nested than they ought to be, e.g. release note markdown
becomes rendered as if it was
|
I think I see what I'm supposed to do instead from |
This is now, I think, correct, unless we expect permalink icons to appear while hovering over section headers in the release notes. I tentatively expected that, and I observe that they do not appear. I think traversal is still correctly assigning fresh ids to sections arising from markdown in the release notes, because the sections appear in the TOC sidebar, and clicking on them still correctly goes to the right url-with-fragment. I think this is where permalinks get created. |
I'm interested in ignoring the prose checks for now as they are numerous and orthogonal to this change. |
If you stick |
To get a permalink, the section has to have a stable identifier. That represents our commitment to keeping it around and keeping that name. Because release notes are basically immutable, this is easy - some mangling of the title and version will suffice. You do this by assigning the |
Perfect, that's exactly the information I was interested in. |
This PR now depends on leanprover/verso#539 to fix markdown header processing to be actually correct. @jrr6 's original implementation was good enough for the typical error explanations markdown, but had iiuc a slightly subtle bug for repeating headers at the same depth, which my PR aims to fix. |
Manual/Meta/Markdown.lean
Outdated
def markdown : PartCommand | ||
| `(Lean.Doc.Syntax.codeblock| ``` $markdown:ident $args*| $txt ``` ) => do | ||
let x <- Lean.Elab.realizeGlobalConstNoOverloadWithInfo markdown | ||
if x != (by exact decl_name%) then Elab.throwUnsupportedSyntax |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this (by exact decl_name%)
tactic invocation appropriate here? As I understand it, I'm not able to literally write ``markdown
because I want to refer to the very def markdown
defined on line 32, right? So this is what I guessed would work instead, having seem decl_name%
many places elsewhere. It does in fact seem to work, and using MessageData.hint
below works to get argument deletion hints as well.
71eb61b
to
6216c69
Compare
Make the type `HeaderMapping` an abbreviation instead of a definition. This change was intended as part of leanprover#539 and will make leanprover/reference-manual#599 easier to land. There's still [one place remaining](https://github.com/leanprover/reference-manual/blob/main/Manual/Meta/ErrorExplanation.lean#L639-L640) in `Manual/Meta/ErrorExplanation.lean` where it's relying on the implementation of this type, which breaks if typeclass resolution doesn't see the `ForIn` instance on `List`. Of course, we could also expose `ForIn` on `HeaderMapping`, or expose a "close all sections for this `HeaderMapping` but I'd rather decouple that decision from [the present PR](leanprover/reference-manual#599).
Make the type `HeaderMapping` an abbreviation instead of a definition. This change was intended as part of #539 and will make leanprover/reference-manual#599 easier to land. There's still [one place remaining](https://github.com/leanprover/reference-manual/blob/main/Manual/Meta/ErrorExplanation.lean#L639-L640) in `Manual/Meta/ErrorExplanation.lean` where it's relying on the implementation of this type, which breaks if typeclass resolution doesn't see the `ForIn` instance on `List`. Of course, we could also expose `ForIn` on `HeaderMapping`, or expose a "close all sections for this `HeaderMapping` but I'd rather decouple that decision from [the present PR](leanprover/reference-manual#599).
Make the type `HeaderMapping` an abbreviation instead of a definition. This change was intended as part of leanprover#539 and will make leanprover/reference-manual#599 easier to land. There's still [one place remaining](https://github.com/leanprover/reference-manual/blob/main/Manual/Meta/ErrorExplanation.lean#L639-L640) in `Manual/Meta/ErrorExplanation.lean` where it's relying on the implementation of this type, which breaks if typeclass resolution doesn't see the `ForIn` instance on `List`. Of course, we could also expose `ForIn` on `HeaderMapping`, or expose a "close all sections for this `HeaderMapping` but I'd rather decouple that decision from [the present PR](leanprover/reference-manual#599).
Previously, we turned all headers into emphasized text regardless of level. With this change, we insert parts into the current verso document.
Now the headers are inserted at the right levels. They don't receive permalinks, which I don't yet understand.
These were reported because of treating markdown sections as first-class parts.
This reverts commit 3a4618a.
9476c5e
to
5e8f3d8
Compare
The one remaining thing holding me back from merging this PR as is is the failing vale prose check. It's unclear to me how I should best ensure I could try wrapping individual blocks with the I'd love to add Any other ideas? I think this actually is a blocker because I believe, reading the workflow file for how vale is invoked, that it's not restricting its attention to changed files. So I conclude that if I break the reference-manual build with this change it's going to stay broken until fixed. |
Discussion out of band led to the following conclusions:
|
Preview for this PR is ready! 🎉 (also as a proofreading version). built with commit 1dfc0db. |
Resolves #542.
Previously, we turned all headers into emphasized text regardless of level. With this change, we insert nested parts into the current verso document corresponding to the markdown structure.