Addition/Deletion Injection Fixes #117
Conversation
bogdancondorachi
force-pushed
the
diffsInjections
branch
from
2fd04be to
8f93f8c
Compare
Pull Request Test Coverage Report for Build 9204549281Warning: 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
💛 - Coveralls |
| @@ -15,6 +15,9 @@ | |||
| { | |||
| public function parse(string $content, Highlighter $highlighter): ParsedInjection | |||
| { | |||
| // Standardize line endings | |||
| $content = preg_replace("/(\r\n|\n|\r)/", PHP_EOL, $content); | |||
There was a problem hiding this comment.
Couldn't we use /\R/ instead of /(\r\n|\n|\r)/?
\R: matches any Unicode newline sequence
|
Looks good! To answer your question:
They used to be so that there aren't any collisions between blade comments 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? |
|
Indeed, using |
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
InlineThemeis not getting theIgnoreTokenTypefor 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.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 😳