-
Notifications
You must be signed in to change notification settings - Fork 37
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
Show diff report in accessible colors and reveal hidden line characters #126
Conversation
088e331
to
b8c69c4
Compare
* wip * wip * wip * wip * wip * wip * wip
Codecov Report
@@ Coverage Diff @@
## master #126 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 16 16
Lines 757 847 +90
=========================================
+ Hits 757 847 +90 |
|
||
@property | ||
def _context_line_count(self) -> int: | ||
return 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous implementation is equivalent to setting this to 0
@@ -28,7 +29,7 @@ class DataSerializer: | |||
|
|||
class MarkerDepthMax: | |||
def __repr__(self) -> str: | |||
return "..." | |||
return SYMBOL_ELLIPSIS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though we use the same symbol for context & max depth, semantically they're different and could theoretically change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but then we could split the symbols apart at that point
count_leading_whitespace: Callable[[str], int] = ( | ||
lambda s: len(s) - len(s.lstrip()) # noqa: E731 | ||
) | ||
if self._context_line_count: | ||
num_space = ( | ||
count_leading_whitespace(lines[self._context_line_count - 1]) | ||
+ count_leading_whitespace(lines[-self._context_line_count]) | ||
) // 2 | ||
else: | ||
num_space = count_leading_whitespace(lines[num_lines // 2]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simpler to reason about but leaves room for ...
having weird indentation
count_leading_whitespace: Callable[[str], int] = ( | |
lambda s: len(s) - len(s.lstrip()) # noqa: E731 | |
) | |
if self._context_line_count: | |
num_space = ( | |
count_leading_whitespace(lines[self._context_line_count - 1]) | |
+ count_leading_whitespace(lines[-self._context_line_count]) | |
) // 2 | |
else: | |
num_space = count_leading_whitespace(lines[num_lines // 2]) | |
mid_line = lines[num_lines // 2] | |
num_space = len(mid_line) - len(mid_line.lstrip()) |
e897763
to
eb6a40b
Compare
is_diff_line = line[0] == "?" | ||
|
||
if is_context_line or is_diff_line: | ||
line = self.__strip_ends(line) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was originally thinking of only showing the new line endings (i.e. the symbols) if necessary. So if the line endings are the only change.
[ | ||
( | ||
"line 0\nline 1\nline 02\nline 3\nline 4\r\nline 5\nline 6\nline 7", | ||
"line 0\nline 1\nline 2\r\nline 3\nline 04\nline 5\nline 6\nline 7", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more cases:
- trailing newlines
- where the difference between 2 sets is extra whitespace (space, tab) at the end
- different number of lines (additions + deletions)
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
🎉 This PR is included in version 0.3.4 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Description
Refactors the diff reporter to improve accessibility and show diff on newlines and carriage returns.
Related Issues
\r\n
a11y
Checklist
Additional Comments
No additional comments.