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

Update chroma to v2 #33

Merged
merged 2 commits into from
Sep 24, 2022
Merged

Update chroma to v2 #33

merged 2 commits into from
Sep 24, 2022

Conversation

jolheiser
Copy link
Contributor

@jolheiser jolheiser commented Jul 7, 2022

This updates chroma to v2.

For context on the below comments, previously this used a version prior to a breaking change.
@zeripath was gracious enough to send a patch that allows the latest version of chroma.

@zeripath
Copy link
Contributor

zeripath commented Jul 26, 2022

Heya @jolheiser how about doing this?

From 50ca7b0882b7c1beadb158f5fa6718db266dad58 Mon Sep 17 00:00:00 2001
From: Andrew Thornton <art27@cantab.net>
Date: Tue, 26 Jul 2022 19:37:22 +0100
Subject: [PATCH] Upgrade to Chroma 2.2.0

Adjust the tests to account for the change PreventSurroundingPre()
---
 go.mod               |  3 ++-
 go.sum               |  7 ++++---
 highlighting_test.go | 10 +++++++++-
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/go.mod b/go.mod
index cd7124a..036deb2 100644
--- a/go.mod
+++ b/go.mod
@@ -3,6 +3,7 @@ module github.com/yuin/goldmark-highlighting
 go 1.13
 
 require (
-	github.com/alecthomas/chroma/v2 v2.1.0
+	github.com/alecthomas/chroma/v2 v2.2.0
+	github.com/dlclark/regexp2 v1.7.0 // indirect
 	github.com/yuin/goldmark v1.4.5
 )
diff --git a/go.sum b/go.sum
index 225e6af..4584097 100644
--- a/go.sum
+++ b/go.sum
@@ -1,12 +1,13 @@
-github.com/alecthomas/chroma/v2 v2.1.0 h1:ZG9L5/RsxO/xIONrBy8Cgo+5si3d9x3osweXc4VHl0o=
-github.com/alecthomas/chroma/v2 v2.1.0/go.mod h1:vf4zrexSH54oEjJ7EdB65tGNHmH3pGZmVkgTP5RHvAs=
+github.com/alecthomas/chroma/v2 v2.2.0 h1:Aten8jfQwUqEdadVFFjNyjx7HTexhKP0XuqBG67mRDY=
+github.com/alecthomas/chroma/v2 v2.2.0/go.mod h1:vf4zrexSH54oEjJ7EdB65tGNHmH3pGZmVkgTP5RHvAs=
 github.com/alecthomas/repr v0.0.0-20220113201626-b1b626ac65ae h1:zzGwJfFlFGD94CyyYwCJeSuD32Gj9GTaSi5y9hoVzdY=
 github.com/alecthomas/repr v0.0.0-20220113201626-b1b626ac65ae/go.mod h1:2kn6fqh/zIyPLmm3ugklbEi5hg5wS435eygvNfaDQL8=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
-github.com/dlclark/regexp2 v1.4.0 h1:F1rxgk7p4uKjwIQxBs9oAXe5CqrXlCduYEJvrF4u93E=
 github.com/dlclark/regexp2 v1.4.0/go.mod h1:2pZnwuY/m+8K6iRw6wQdMtk+rH5tNGR1i55kozfMjCc=
+github.com/dlclark/regexp2 v1.7.0 h1:7lJfhqlPssTb1WQx4yvTHN0uElPEv52sbaECrAQxjAo=
+github.com/dlclark/regexp2 v1.7.0/go.mod h1:DHkYz0B9wPfa6wondMfaivmHpzrQ3v9q8cnmRbL6yW8=
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
diff --git a/highlighting_test.go b/highlighting_test.go
index 9eb9a98..1b8e96e 100644
--- a/highlighting_test.go
+++ b/highlighting_test.go
@@ -428,6 +428,14 @@ LINE8
 	}
 }
 
+type nopPreWrapper struct{}
+
+// Start is called to write a start <pre> element.
+func (nopPreWrapper) Start(code bool, styleAttr string) string { return "" }
+
+// End is called to write the end </pre> element.
+func (nopPreWrapper) End(code bool) string { return "" }
+
 func TestHighlightingLinenos(t *testing.T) {
 	outputLineNumbersInTable := `<div class="chroma">
 <table class="lntable"><tr><td class="lntd">
@@ -464,7 +472,7 @@ func TestHighlightingLinenos(t *testing.T) {
 						WithFormatOptions(
 							chromahtml.WithLineNumbers(test.lineNumbers),
 							chromahtml.LineNumbersInTable(test.lineNumbersInTable),
-							chromahtml.PreventSurroundingPre(true),
+							chromahtml.WithPreWrapper(nopPreWrapper{}),
 							chromahtml.WithClasses(true),
 						),
 					),
-- 
2.37.1

@jolheiser
Copy link
Contributor Author

@zeripath Thanks for the patch! I've applied it, so now this is using the latest version of chroma.

CC @yuin

@yuin
Copy link
Owner

yuin commented Aug 3, 2022

This PR breaks goldmark-highlighting compatibility as well.
So This should be v2.

@jolheiser
Copy link
Contributor Author

This PR breaks goldmark-highlighting compatibility as well. So This should be v2.

Fine with me. Do you want me to mark this go.mod as v2? Do you want to make a v2 branch, or make master point to v2 and have a separate v1 branch?

@yuin
Copy link
Owner

yuin commented Sep 23, 2022

@jolheiser Sorry for late reply.
I've pushed the v2 branch into this repository. Could you rebase your branch and change PR target branch to the v2.

  • TODO
    • merge your PR
    • update README for v1 and v2
    • change the default branch to v2

jolheiser and others added 2 commits September 23, 2022 11:07
Adjust the tests to account for the change PreventSurroundingPre()

Signed-off-by: jolheiser <john.olheiser@gmail.com>
Adjust the tests to account for the change PreventSurroundingPre()
@jolheiser
Copy link
Contributor Author

@yuin I've rebased onto v2 and updated. (It appears I mucked the commit messages a bit, but should be fine)

@yuin yuin merged commit 8b60e41 into yuin:v2 Sep 24, 2022
@yuin
Copy link
Owner

yuin commented Sep 24, 2022

@jolheiser
I've updated READMEs and made v2 a default branch. Thanks for your contribution.

lunny pushed a commit to go-gitea/gitea that referenced this pull request Sep 26, 2022
The behaviour of `PreventSurroundingPre` has changed in
alecthomas/chroma#618 so that apparently it now
causes line wrapper tags to be no longer emitted, but we need some form
of indication to split the HTML into lines, so I did what
yuin/goldmark-highlighting#33 did and added the
`nopWrapper`.

Maybe there are more elegant solutions but for some reason, just
splitting the HTML string on `\n` did not work.

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
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.

3 participants