tempestphp / highlight

🎨 Fast, extensible, server-side code highlighting for web and terminal
https://tempest.stitcher.io/highlight/01-getting-started
MIT License
640 stars 38 forks source link

Addition/Deletion Injection Fixes #117

Closed bogdancondorachi closed 5 months ago

bogdancondorachi commented 5 months ago

Hi @brendt, this is a continuation of fixes presented in #104 where addition/deletion is not able to get the line endings correctly in my use case scenario, similar to #115 on the gutters it cannot read the current line thus applying it always on the first line.

What I did was standardize the line endings to EOL so it can be more flexible when handling line endings.

$content = preg_replace("/(\r\n|\n|\r)/", PHP_EOL, $content);

While I was debugging this I've notice that the InlineTheme is not getting the IgnoreTokenType for Addition/Deletion endings, being unable to replace them with '{-' and '-}'. So I've fixed it by returning the class when it's not found in the theme file instead of returning an empty span. This also return the classes of highlighting tags in case that are not present in the theme file.

In addition to the last PR, I've removed the spacing after <span class="hl-gutter "> if it does not follow by another class.

image

Also if you could explain the use of IgnoreTokenType, disabling it even solves this InlineTheme problem and in all my testing I could not find any scenario that it's helping in any way, results being the same with or without. Probably it has a meaning, I was just wondering if it's really necessary or if we could find another solution.

P.S. Sorry for the unit-tests I always fucked them 😳

coveralls commented 5 months ago

Pull Request Test Coverage Report for Build 9204549281

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/Themes/InlineTheme.php 0 1 0.0%
<!-- Total: 4 5 80.0% -->
Files with Coverage Reduction New Missed Lines %
src/Languages/Php/PhpDocCommentLanguage.php 1 93.33%
<!-- Total: 1 -->
Totals Coverage Status
Change from base Build 9013465686: 0.1%
Covered Lines: 1497
Relevant Lines: 1569

💛 - Coveralls
brendt commented 5 months ago

Looks good! To answer your question:

Also if you could explain the use of IgnoreTokenType

They used to be so that there aren't any collisions between blade comments {{-- and ignore tags {{-.

However, it's more than possible that this now also works without these tokens, because of other changes that have been made. I'll look into this separately, let's keep this PR focussed on one thing :)

I'm fine merging this in, but could you please explain that one question about the regex differences?

bogdancondorachi commented 5 months ago

Indeed, using '/\R/' is more comprehensive and handles all cases in one go including unicode. Also being inlined with #115

brendt commented 5 months ago

Tagged: https://github.com/tempestphp/highlight/releases/tag/2.4.5