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

Add basic support for syntax highlighting in code blocks #515

Merged
merged 6 commits into from
Dec 15, 2022

Conversation

hloeung
Copy link
Collaborator

@hloeung hloeung commented Nov 29, 2022

As it is right now, it will only use syntax highlighting on code blocks where it specifies syntax highlighting, that is ```blah. This is because it's a very basic implementation where we're doing per-line.

cr8XiZmV8b

Ideally, we'll want to buffer up code block lines and then pass it through to https://github.com/alecthomas/chroma which will then analyse and try to determine what style to use if none provided.

@hloeung
Copy link
Collaborator Author

hloeung commented Nov 29, 2022

Added workaround to alecthomas/chroma#716 for now.

@hloeung hloeung force-pushed the syntax-highlighting branch 2 times, most recently from b2cd02d to d1782d1 Compare November 29, 2022 05:23
@hloeung hloeung requested a review from 42wim December 1, 2022 04:49
@hloeung hloeung marked this pull request as ready for review December 1, 2022 04:49
@hloeung hloeung changed the title [WIP] Add basic support for syntax highlighting in code blocks Add basic support for syntax highlighting in code blocks Dec 1, 2022
Copy link
Owner

@42wim 42wim left a comment

Choose a reason for hiding this comment

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

That looks pretty cool :)
Some small remarks:

  • not necessary for this PR, but for the next PR I think it's maybe better to move the codeblock handling to a separate method.
  • what are your thoughts about instead of making 3 extra options using just one like eg SyntaxHighlighting="formatter:style" if it's empty it's not used and otherwise get the formatter/style out of it ?

@hloeung
Copy link
Collaborator Author

hloeung commented Dec 3, 2022

  • what are your thoughts about instead of making 3 extra options using just one like eg SyntaxHighlighting="formatter:style" if it's empty it's not used and otherwise get the formatter/style out of it ?

Sounds good, updated PR implementing this - 3 extra options down to just one.

Thanks for your review.

@hloeung hloeung requested a review from 42wim December 3, 2022 21:52
mm-go-irckit/userbridge.go Outdated Show resolved Hide resolved
@hloeung hloeung marked this pull request as draft December 3, 2022 23:49
@42wim 42wim added this to the v0.27.0 milestone Dec 3, 2022
@hloeung
Copy link
Collaborator Author

hloeung commented Dec 4, 2022

Okay, ready to go / review. Thanks and much appreciated.

@hloeung hloeung marked this pull request as ready for review December 4, 2022 00:21
@hloeung hloeung requested a review from 42wim December 4, 2022 00:21
@hloeung hloeung force-pushed the syntax-highlighting branch 2 times, most recently from 7571b49 to 328251d Compare December 4, 2022 02:58
mm-go-irckit/userbridge.go Show resolved Hide resolved
mm-go-irckit/userbridge.go Show resolved Hide resolved
mm-go-irckit/userbridge.go Show resolved Hide resolved
Copy link
Owner

@42wim 42wim left a comment

Choose a reason for hiding this comment

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

LGTM

@hloeung hloeung merged commit d6583b6 into 42wim:master Dec 15, 2022
@hloeung hloeung deleted the syntax-highlighting branch December 15, 2022 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants