Styled markdown, with a few tweaks #51928
Conversation
tecosaur
force-pushed
the
styled-markdown
branch
from
0bfb3b2 to
453a91b
Compare
tecosaur
added
display and printing
|
What do you know, all the dependent PRs are now merged 🥳. We can now think about reviewing and merging this. |
tecosaur
force-pushed
the
styled-markdown
branch
2 times, most recently
from
c2d7340 to
8639b74
Compare
|
@vtjnash any further thoughts on the current version? |
| elseif md.language == "julia-repl" || Base.startswith(md.language, "jldoctest") | ||
| hl = AnnotatedString(md.code) | ||
| for (; match) in eachmatch(r"(?:^|\n)julia>", hl) | ||
| StyledStrings.face!(match, :repl_prompt_julia) |
There was a problem hiding this comment.
This seems a bit suspect to imply that applying a face! to the result of eachmatch will alter the string hl
There was a problem hiding this comment.
When taking a substring of an AnnotatedString, the underlying string is not mutable, but the annotations are. This is rather convenient when you want to search for patterns to style (no other convenient patterns are currently possible, actually).
| hl = AnnotatedString(md.code) | ||
| for (; match) in eachmatch(r"(?:^|\n)julia>", hl) | ||
| StyledStrings.face!(match, :repl_prompt_julia) | ||
| afterprompt = match.offset + match.ncodeunits + 1 |
There was a problem hiding this comment.
There does not appear to be a field ncodeunits (it is a function)
There was a problem hiding this comment.
The match is of type SubString{AnnotatedString}, so it indeed has a field ncodeunits. May as well use the function though.
| for (; match) in eachmatch(r"(?:^|\n)julia>", hl) | ||
| StyledStrings.face!(match, :repl_prompt_julia) | ||
| afterprompt = match.offset + match.ncodeunits + 1 | ||
| _, exprend = Meta.parse(md.code, afterprompt, raise = false) |
There was a problem hiding this comment.
It feels a bit tricky to have a full REPL here, but anyways, I think this is supposed to use Base.parse_input_line aka Meta.parseall here and repeatedly add lines until the expression is complete. The result may not be quite the same as eachmatch would return, but may be closer to what eachline returns, followed by if ismatch followed by Meta.parseall consuming more lines until it reaches a concluding statement (a blank line or a complete expression or a parse error)
There was a problem hiding this comment.
This is modelled after the current behaviour of OhMyREPL which seems to work. I think Kristoffer and I had a brief conversation on this on Slack where I asked "why Meta.parse not Meta.parseatom?" but I forget the details and it's been swallowed by the Slack-hole at this point 🥲.
I'll also note that it's actually somewhat difficult to tell the behavioural difference between parse, parseatom, and parseall as the latter two have no docstrings and just call Core._parse, which itself seems to have no (useful) comments.
| function term(io::IO, md::MarkdownElement, columns) | ||
| a = IOContext(AnnotatedIOBuffer(), io) | ||
| term(a, md, columns) | ||
| print(io, read(seekstart(a.io), AnnotatedString)) |
There was a problem hiding this comment.
Isn't there an optimized version of this, for copying between IO objects without the intermediate copy?
| print(io, read(seekstart(a.io), AnnotatedString)) | |
| write(io, seekstart(a.io)) |
Or is the problem that io might be wrapped up somewhat, so that dispatch currently won't reliably know if it is annotation-capable and end up discarding those?
There was a problem hiding this comment.
io may be stdout, and we currently don't have a specialised print/write method for AnnotatedIOBuffer to .
I'd suggest that we perhaps add a "TODO change to direct write after implementing specialised AnnotatedIOBuffer printing"-type mental note or comment so we can come back to this if/when we do so.
| i < lastindex(L) && println(io) | ||
| code = if md.language ∈ ("", "julia") | ||
| highlight(md.code) | ||
| elseif md.language == "julia-repl" || Base.startswith(md.language, "jldoctest") |
There was a problem hiding this comment.
Actually, could you drop this specific particular part of the PR, merge everything else, and then add this in a later PR? It is a bit of a bonus feature, and I don't want it to hold up everything else
(perhaps just add it to the list along with "julia" to get some primitive highlighting, or just leave it bare for now)
There was a problem hiding this comment.
Sure, I'll bundle it with julia for now.
There was a problem hiding this comment.
For future reference, this is the code being removed:
elseif md.language == "julia-repl" || Base.startswith(md.language, "jldoctest")
hl = AnnotatedString(md.code)
for (; match) in eachmatch(r"(?:^|\n)julia>", hl)
StyledStrings.face!(match, :repl_prompt_julia)
afterprompt = match.offset + ncodeunits(match) + 1
_, exprend = Meta.parse(md.code, afterprompt, raise = false)
highlight!(hl[afterprompt:prevind(md.code, exprend)])
if (nextspace = findnext(' ', md.code, exprend)) |> !isnothing
nextword = hl[exprend:prevind(hl, nextspace)]
if nextword == "ERROR:"
StyledStrings.face!(nextword, :error)
end
end
end
hl
tecosaur
force-pushed
the
styled-markdown
branch
from
8639b74 to
4d1ffb4
Compare
| print(io, ' '^margin, L[i]) | ||
| i < lastindex(L) && println(io) | ||
| end | ||
| code = if md.language ∈ ("", "julia", "julia-repl", "jldoctest") |
There was a problem hiding this comment.
| code = if md.language ∈ ("", "julia", "julia-repl", "jldoctest") | |
| code = if md.language ∈ ("julia", "julia-repl", "jldoctest") |
just arbitrary <raw> blocks marked with ``` shouldn't get julia-formatted, right?
There was a problem hiding this comment.
I guess it depends how you interpret them. I tend to see them as "whatever the default language is" which on GitHub is text and in Julia docstrings is julia.
Ultimately, it's a heuristic choice, and I may well have gone the wrong way here.
There was a problem hiding this comment.
I think raw is the conservative choice here to stick with
There was a problem hiding this comment.
Sure. It would be nice to syntax highlight function signatures though, which usually appear like so:
"""
annotate!(char::AnnotatedChar, label::Symbol => value)
Annotate `char` with the pair `label => value`.
"""There was a problem hiding this comment.
True. Probably should be a separate PR though, since it is potentially a more significant change that downstream consumers would not be able to distinguish between annotated and un-annotated content
There was a problem hiding this comment.
Yes, the initial code element of the docstring would be highlighted as Julia code, if that's what you mean.
There was a problem hiding this comment.
@KristofferC might have some helpful comments here, since OhMyREPL does Julia syntax highlighting of markdown code blocks.
There was a problem hiding this comment.
It seems like we would need a way to distinguish arbitrary Markdown that is being formatted from specifically Julia docstrings that are being rendered
There was a problem hiding this comment.
The only way to conclusively tell, is to use language annotations on code blocks. Short of that, it's a guess/default. I think "guess Julia because we're in Julia" and requiring raw/text to be explicit makes sense, but as you demonstrated earlier that's not the only reasonable position.
There was a problem hiding this comment.
If defaulting to rendering Julia syntax gives us highlighting in the docstrings and otherwise not, then I think we should default to Julia.
tecosaur
force-pushed
the
styled-markdown
branch
from
bd47dc8 to
8151af1
Compare
|
@vtjnash you added a "merge me" label but did not approve the PR... Should this be read as an "implicit approval"? |
|
There seem to be genuine errors in the CI job logs |
tecosaur
force-pushed
the
styled-markdown
branch
2 times, most recently
from
008be87 to
3927f8e
Compare
|
The Pkg CI errors will be resolved once JuliaLang/Pkg.jl#3884 is merged, and Pkg bumped. |
tecosaur
force-pushed
the
styled-markdown
branch
from
3927f8e to
c12142f
Compare
It's convenient for dispatch.
Using StyledStrings for styled printing has a number of benefits, including but not limited to: - Italics "just working" on terminals that announce support - Functioning links, for the first time - Greater compossibility of rendered markdown content - Customisability of the printing style Then with JuliaSyntaxHighlighting, we get support for syntax-highlighted Julia code too.
The spacing between list items might as well represent whether the list is a tight or loose list.
It seems to make sense not to treat everything other than "rst" as Julia. We may as well follow the same heuristics as the terminal rendering for consistency.
Since we're already using StyledStrings for rendering Julia in the terminal, we can also handle "styled"-labelled code blocks fancily, thanks to the `styled` function provided by StyledStrings. In all non-terminal contexts, the styling metadata is simply discarded, but could be used in the future (for instance StyledStrings currently supports HTML output too).
In the course of the markdown PR, an issue with the use of deepcopy in StyledStrings was revealed. This has now been fixed, and to obtain the fix StyledStrings is bumped.
|
We might be in the clear now! 🎉 🤞 |
|
If there are no objections, I'll merge this shortly. |
|
Maybe just a quick check to do if you have the branch compiled: do MarkdownAST and Documenter test suites pass with this? |
|
Thanks for that suggestion Morten, I do indeed and the results are:
|
Interesting, but that seem completely unrelated, and so I think we can ignore that. Documenter's reliance on the Markdown stdlib should mostly be covered by the MarkdownAST tests anyway. Thanks for checking! |
|
Since it's been a while, Triage thinks we should just let this appear in 1.12. |
Fixes #54399 by re-introducing the code seperated out from the styled Markdown PR at Jameson's request (#51928 (comment)). The code itself is modelled after [equivalent code in OhMyREPL](https://github.com/KristofferC/OhMyREPL.jl/blob/b0071f5ee785a81ca1e69a561586ff270b4dc2bb/src/MarkdownHighlighter.jl#L15-L31). The new `markdown_julia_prompt` face allows people to make the "prompt" shown in Markdown code visually distinct, to [avoid confusing it with the REPL prompt at a glance](KristofferC/OhMyREPL.jl#100). By way of example, I make it italic by augmenting my `faces.toml` with ```toml [markdown] julia_prompt = { italic = true } ```
Fixes JuliaLang#54399 by re-introducing the code seperated out from the styled Markdown PR at Jameson's request (JuliaLang#51928 (comment)). The code itself is modelled after [equivalent code in OhMyREPL](https://github.com/KristofferC/OhMyREPL.jl/blob/b0071f5ee785a81ca1e69a561586ff270b4dc2bb/src/MarkdownHighlighter.jl#L15-L31). The new `markdown_julia_prompt` face allows people to make the "prompt" shown in Markdown code visually distinct, to [avoid confusing it with the REPL prompt at a glance](KristofferC/OhMyREPL.jl#100). By way of example, I make it italic by augmenting my `faces.toml` with ```toml [markdown] julia_prompt = { italic = true } ```
With
StyledStringsandJuliaSyntaxHighlightingwe can make the rendering ofMarkdown.MD:Since this is mostly about rendering, here are some screenshots:
Current
New default
New customised
Depends on #51807, #51810, and #51869