-
Notifications
You must be signed in to change notification settings - Fork 7
fix: Add HTML entity replacements for tab and space characters #10
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
base: main
Are you sure you want to change the base?
fix: Add HTML entity replacements for tab and space characters #10
Conversation
|
@LaurentAerens it seems like this would be a valuable change. how do you feel about proposing it for all of the libraries so that the C# implementation doesn’t work differently than the others? Further, and this could be a follow-up enhancement, but it seems like this could be moved into a function specific to transforming the display of the diffs for pretty HTML viewing. That might include other changes such as replacing C0 controls with their visual representations, e.g. display “␛” instead of displaying the raw byte 0x1B. Having a separate function could ease the cross-language consistency and provide a good place to discuss different representations. |
|
I've updated it in all remaining languages. |
|
i'm unsure about the merge conflicts, i don't see any so i'm a bit confused about what's going on. |
|
@LaurentAerens aha, we converted the JS library to an |
|
@dmsnell i think i'm having an issue pulling the lastest version of main, so if you can do it for now that would be amazing. i'm going to investigate why i can't pull the lastest version. |
|
@LaurentAerens I have pushed eb4a437. You should be able to in review I verified that browsers are going to render to that end, however, I think there’s a potential issue with this patch in that it might prevent word-wrapping for diff elements with long streams of text. can you try viewing a pretty diff in a browser where there’s a sufficiently long span of inserted/deleted/unchanged text and see if it wraps? if we have that problem it may be worth it to use to limit the replacement of the tabs and spaces with those only on the leading or trailing end of a string, something that I would think should be accomplishable with a tiny change to the regex patterns. |
diff_prettyHtmlcurrently doesn't highlight leading/trailing space differences. Because spaces in HTML aren't visible.This pull request changes that.
Instead of
diff_prettyHtmlnow returnsresulting in differences with leading/training spaces being visible in HTML.
Example:
When we compare "Test" and "Test " (notice the second string doesn't equal the first string since it has a trailing space).
Previously looked like this:
Now with this pull requests it looks like this:
Note: this issue also seems to be present in the JavaScript implementation: google#144