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

Remove newline when highlighting random chunks of code #12180

Merged
merged 4 commits into from
Jul 7, 2020

Conversation

mrsdizzie
Copy link
Member

@mrsdizzie mrsdizzie commented Jul 7, 2020

By default Chroma will always add new lines for any pieces of code for certain file formats. See :

https://github.com/alecthomas/chroma/blob/bac6996317c811706b2ade529f31d276a1accae8/regexp.go#L433-L435

And

https://github.com/alecthomas/chroma/blob/bac6996317c811706b2ade529f31d276a1accae8/lexers/m/make.go#L16

This breaks the way we render diff when one of these formats also has a line of added/deleted code within a new/del line. Make sure this doesn't happen by overriding the default lexer options and setting the Nested flag to true.

Fixes #12172

Somewhere when tokenizing a newline gets added to code formatted by chroma. This breaks the case of 'added-code' inside of an 'added-line' in a diff. Just remove any newline when processing chunks of code since we don't need it.

Fixes go-gitea#12172
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jul 7, 2020
@lafriks lafriks added type/bug and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 7, 2020
@lafriks lafriks added this to the 1.13.0 milestone Jul 7, 2020
@lafriks lafriks added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Jul 7, 2020
@silverwind
Copy link
Member

silverwind commented Jul 7, 2020

Tokens never span across multiple lines, right? Should probably report this to chroma.

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jul 7, 2020
@mrsdizzie
Copy link
Member Author

Correct and it should never see more than one real line of code at a time anyway because of how we handle diffs and process them one line at a time.

I think this might be intended chroma behavior and this is just a separate case on our end that I didn't fully account for where we process one line of code in sections.

I'll look further to see if something on their end needs to change, but this will fix the problem for us.

@mrsdizzie mrsdizzie changed the title Remove newline when highlighting random chunks of code WIP: Remove newline when highlighting random chunks of code Jul 7, 2020
@mrsdizzie
Copy link
Member Author

actually see another issue need to fix first before merging

@mrsdizzie mrsdizzie changed the title WIP: Remove newline when highlighting random chunks of code Remove newline when highlighting random chunks of code Jul 7, 2020
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 7, 2020
@mrsdizzie
Copy link
Member Author

@silverwind for a better answer and why I didn't notice when testing:

Whats actually happening here is that only certain lexers are configured to always add a new line for code when there isn't one there. See:

https://github.com/alecthomas/chroma/blob/bac6996317c811706b2ade529f31d276a1accae8/regexp.go#L433-L435

And

https://github.com/alecthomas/chroma/blob/bac6996317c811706b2ade529f31d276a1accae8/lexers/m/make.go#L16

This is inherited from Pygments and the idea is that the lexers aren't able to parse arbitrary lines of code for certain formats without a newline since it is part of the matching. So thats why it happens in the combination example case of being a Makefile AND having a section of added/deleted code within the diff line.

We don't need the newline after the code has been processed so OK to remove it. Added check to not process a line that is only a newline as well to preserve those

@silverwind
Copy link
Member

This is inherited from Pygments and the idea is that the lexers aren't able to parse arbitrary lines of code for certain formats without a newline since it is part of the matching

I guess we could get a better highlighting result by passing the diff's before/after as multi-line at the cost of reduced performance of course. The current diff highlighting still seems good so maybe not worth the effort.

@lafriks
Copy link
Member

lafriks commented Jul 7, 2020

🚀

@lafriks lafriks merged commit 078d2fb into go-gitea:master Jul 7, 2020
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
* Remove newline when highlighting random chunks of code

Somewhere when tokenizing a newline gets added to code formatted by chroma. This breaks the case of 'added-code' inside of an 'added-line' in a diff. Just remove any newline when processing chunks of code since we don't need it.

Fixes go-gitea#12172

* don't process empty lines

* This is the proper way to fix this by telling chroma not to add the newline in the first place
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Highlighted diff emitting extraneous newlines
4 participants