search

yuin / goldmark-highlighting

A Syntax highlighting extension for the goldmark markdown parser.
MIT License
100 stars 14 forks source link

Misc changes #10

Closed bep closed 4 years ago

bep commented 4 years ago
bep commented 4 years ago

OK; I think this should make sense now. It still needs the Chroma PR and could probably do with a squash.

One question: Is this superfluous? I don't see it getting invoked:

https://github.com/yuin/goldmark-highlighting/blob/631f3f291c26dd1ae887ff4d07198255d49d05ab/highlighting.go#L149

yuin commented 4 years ago

You need to implement SetOption . SetOption is called from here.

bep commented 4 years ago

I have updated the first comment with the changes made and squashed the commits.

So, this now works for me, but it should be improved to make it easier to use, ref.:

https://github.com/yuin/goldmark-highlighting/blob/34ab6e256c83f80b202f6d39cefd9de5ed5e1a14/highlighting.go#L465

The above logic will break if you also enable line numbers with WithFormatOptions.

Maybe depending on the outcome of https://github.com/alecthomas/chroma/issues/301 -- but I suggest you change WithFormatOptions to take a custom struct that allows toggling of options.

I can make this change as part of this PR (this will then make the WithLineNumbers and WithLineNumbersInTable superfluous) if you want, or you/we could do it later.

bep commented 4 years ago

I can make this change as part of this PR (this will then make the WithLineNumbers and WithLineNumbersInTable superfluous) if you want, or you/we could do it later.

Second thought, that would make a little bit too much duplication. I suggest we keep it as in this PR for now and you can consider changing it if the Chroma issue somehow gets accepted.

bep commented 4 years ago

Seems that https://github.com/alecthomas/chroma/issues/301 is going to be accepted. I will fix that first, which should make this plenty simpler.

bep commented 4 years ago

@yuin Chroma has tagged a new release, so this PR is ready to be considered for a merge. See the first comment for a list of changes.

yuin commented 4 years ago

@bep I take a quick look of this, it seems many unnecessary dependencies have been added to the go.sum . Could you give a try to go mod tidy? .

bep commented 4 years ago

Could you give a try to go mod tidy?

Just did.

I originally did:

go get -u github.com/alecthomas/chroma

To get the new Chroma. I now see I need to rebase or something.

bep commented 4 years ago

I have rebased and tidy'ed, but there are still many new go.sum entries. I don't know how to avoid that and get the updated Chroma.

yuin commented 4 years ago

@bep, OK. Changes almost no problem. I see 1 wrong comment. Could you fix it?