Skip to content
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

Introduce new syntax highlighting stdlib #51810

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

tecosaur
Copy link
Contributor

@tecosaur tecosaur commented Oct 21, 2023

Having this as a standard library allows for syntax highlighting to be used in the Markdown and REPL standard libraries. As a bonus, this provides an easy and canonical way of obtaining syntax highlighted code across the ecosystem.

This is a pretty tiny library, at just ~150 lines, but a useful one.

@tecosaur
Copy link
Contributor Author

I imagine there's going to be some bikeshedding of the highlight rules in https://github.com/JuliaLang/JuliaSyntaxHighlighting.jl/blob/main/src/JuliaSyntaxHighlighting.jl 🙂

To kick things off, here's a screenshot of what the current scheme looks like:

image


That said, if the basic idea/system here looks ok, we could also merge this pre-styling-bikeshedding so that bikeshedding can occur concurrently with usage in Markdown.

@tecosaur tecosaur added display and printing Aesthetics and correctness of printed representations of objects. stdlib Julia's standard library labels Oct 21, 2023
@tecosaur
Copy link
Contributor Author

CI failure seems to be because tests are expected which is ... entirely reasonable. I just thought I'd wait for initial comments before adding tests.

@LilithHafner
Copy link
Member

Ideally, this and the Julia vscode extension would use the same formatting backend (calling out julia-vscode for special attention because it is the most popular Julia editing environment)

I think this is the current source of truth for syntax highlighting in julia-vscode

cc @pfitzseb

If the approach in JuliaSyntaxHighlighting.jl is vastly simpler than the one in julia-vscode and julia-vscode can't depend on Julia for syntax highlighting, then I suppose we may have to keep then separate. Even then I would lean toward exactly mimicking the VSCode extension's formatting choices, at least at first.

Also, stdlibs should have CI.

Those aside, and this lgtm. I definitely support the overall direction of adding syntax highlighting as a stdlib.

If REPL ever stops being a stdlib, then this can go with it. @vchuravy, are there any steps we should take to make this stdlib "free" or does that happen by default?

@pfitzseb
Copy link
Member

The VS Code extension can additionally support highlighting provided by the language server, but that can't be the only source for highlighting. A simple regex based grammar is still required as a baseline (note that the repo you linked also powers syntax highlighting on GitHub).

It's also worth noting that the textmate grammar emits more fine grained scopes than JuliaSyntaxHighlighting.jl does currently, e.g. keyword.other.option-toggle.regexp.julia or keyword.operator.relation.types.julia. That is by no means required, but can come in handy for more advanced or julia specific themes.

Regarding the styling in that screenshot: Generally looks really good to me, but the julia_type styling for head::JuliaSyntax.SyntaxHead seems pretty unfortunate imho.

@tecosaur
Copy link
Contributor Author

Thanks, Sebastian. I'm not familiar with the VS Code extension, so I appreciate hearing from somebody who is when it's referenced.

It's also worth noting that the textmate grammar emits more fine grained scopes than JuliaSyntaxHighlighting.jl does currently, e.g. keyword.other.option-toggle.regexp.julia or keyword.operator.relation.types.julia. That is by no means required, but can come in handy for more advanced or julia specific themes.

I'll see if we can get relation-ness of operators from JuliaSyntax.

Macro-specific highlighting seems like it could be possible, but might be something to work on after merging this initial version (and is something I think would be nice with styled"" too).

Regarding the styling in that screenshot: Generally looks really good to me, but the julia_type styling for head::JuliaSyntax.SyntaxHead seems pretty unfortunate imho.

Could you elaborate on unfortunate?

@pfitzseb
Copy link
Member

I'll see if we can get relation-ness of operators from JuliaSyntax.

I'm not saying we should do that, FWIW. And if we do, not sure if it's worth looking at the scopes used in the textmate grammar, because those are very arbitrary and organically grown.

Could you elaborate on unfortunate?

I'd want the SyntaxHead part to be highlighted as a type as well, not just the module. Same goes for the parametric type there.

@tecosaur
Copy link
Contributor Author

I'd want the SyntaxHead part to be highlighted as a type as well, not just the module. Same goes for the parametric type there.

Ah yes, this has actually slightly bugged me too. I'll give it another look.

@tecosaur
Copy link
Contributor Author

Ok, I think I've got the type highlighting behaving better with this new rule:

elseif lastk  (K".", K"{") && last2f == :julia_type
    :julia_type

image

@tecosaur
Copy link
Contributor Author

[me] I'll see if we can get relation-ness of operators from JuliaSyntax.

Erm, I think this is already done.

image

Though it should probably be renamed to julia_comparator.

@tecosaur
Copy link
Contributor Author

Oh, I see you actually referenced the type relation category, is that perhaps for >: and <:? The regex is ... not nice to read.

@pfitzseb
Copy link
Member

pfitzseb commented Oct 26, 2023

Oh, I see you actually referenced the type relation category, is that perhaps for >: and <:? The regex is ... not nice to read.

Yes, it is.

Generally, the textmate grammar has more specificity. If you scroll down a bit, you'll see keyword.operator.applies.julia, keyword.operator.relation.in.julia, keyword.operator.transposed-variable.julia, and more. So you can color a in b differently from x[begin:end], even though all of those are keywords (for some definition of keyword).

But yeah, as I said before: I don't suggest following atom-language-julia's example here (at least when it comes to the actual implementation).

@@ -4,9 +4,9 @@ STDLIBS_WITHIN_SYSIMG := \

INDEPENDENT_STDLIBS := \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LilithHafner to answer your question. As long as the stdlib is here it is "free", but not yet upgrade-able.

Upgrade-able requires registering in the Registry and Pkg allowing it to update.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Is there anything that should be done on the JuliaSyntaxHighlighting side of things to enable Pkg to allow it to upgrade?

@tecosaur
Copy link
Contributor Author

tecosaur commented Nov 9, 2023

Currently listed CI failures seem spurious (https://buildkite.com/julialang/julia-master/builds/29086), I'll try updating the branch to see if that changes.

@tecosaur tecosaur force-pushed the jlsyntaxhighlight-stdlib branch from 5a46a57 to ec9b978 Compare November 9, 2023 02:42
@tecosaur
Copy link
Contributor Author

Ok, beyond the particular syntax highlighting rules (which I think we can discuss and refine further in the stdlib?), are there any larger aspects to this that need to be discussed/reviewed at this stage?

@LilithHafner
Copy link
Member

Ironic combination of issues:

  1. The stdlib doesn't have any tests. I think that's blocking.
  2. All tests fail in CI. That's definitely blocking.

Having this as a standard library allows for syntax highlighting to be
used in the Markdown and REPL standard libraries. As a bonus, this
provides an easy and canonical way of obtaining syntax highlighted code
across the ecosystem.
@tecosaur tecosaur force-pushed the jlsyntaxhighlight-stdlib branch from ec9b978 to 67f5e9b Compare December 29, 2023 12:07
@tecosaur
Copy link
Contributor Author

I've just updated the stdlib to have some basic docs and tests.

@tecosaur tecosaur added the feature Indicates new feature / enhancement requests label Dec 29, 2023
@tecosaur
Copy link
Contributor Author

CI failure looks unrelated.

@tecosaur tecosaur added the awaiting review PR is complete and seems ready to merge. Has tests and news/compat if needed. CI failures unrelated. label Jan 12, 2024
@tecosaur
Copy link
Contributor Author

I don't think there's anything holding this back now? Might anybody be able to spare the time to review this for merging?

@LilithHafner LilithHafner merged commit e74f9e4 into JuliaLang:master Jan 24, 2024
7 of 9 checks passed
@LilithHafner LilithHafner removed the awaiting review PR is complete and seems ready to merge. Has tests and news/compat if needed. CI failures unrelated. label Jan 24, 2024
@tecosaur
Copy link
Contributor Author

🥳 thanks a lot Lilith, it's great to see some movement on #50817 again 😀

@LilithHafner
Copy link
Member

Happily! Ideally the stdlib should have CI (JuliaLang/JuliaSyntaxHighlighting.jl#1), which I think is why I didn't merge this last time I glanced at it. However,

  • I should have said that explicitly because my previous comment said "tests" not CI
  • CI on this PR (and now all PRs to julialang/julia) does run the stdlib tests so that's not a blocker.

@tecosaur
Copy link
Contributor Author

Yea, CI on the repo itself and a build of the docs would be nice to have, I just didn't think they were a barrier to merging.

@tecosaur tecosaur deleted the jlsyntaxhighlight-stdlib branch January 25, 2024 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
display and printing Aesthetics and correctness of printed representations of objects. feature Indicates new feature / enhancement requests stdlib Julia's standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants