-
Notifications
You must be signed in to change notification settings - Fork 23
feat: add stablePatchmarks option #72
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
feat: add stablePatchmarks option #72
Conversation
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.
Looks like it's heading i the right direction :). Left some online comments to address
src/index.js
Outdated
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 think we can make this regex better and take into account that we have two @@ and numbers with commas between. Also I'm leaning towards replacing them with:
@@ --- --- @@
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.
Also we only need to check it only if expand === false
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.
👍
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 considered a more rigorous regex, but since jest-diff seems to always pad the start of the line with whitespace, I went the lazy route. But I agree this should be improved. Here's an example of the new regex: https://regex101.com/r/KLDkC0/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.
Ha, left out the 0s in the number range. Updated regex: https://regex101.com/r/KLDkC0/2
README.md
Outdated
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.
How about stablePatchmarks? I'm still not sure how to best me it. @SimenB ideas?
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.
stable is better than omit. Other possible verbs:
normalize
scrub
normalize doesn't communicate a whole lot. scrub communicates what we are doing to the patch mark, while stable communicates the purpose behind it.
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 think I'd still go with stable
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.
all set
README.md
Outdated
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.
Would also be cool to show an example like: changes @@ -1,1 +1,2 @@ to @@ --- --- @
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.
done
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.
One last thing and I'm good :)
Solves #69.
I'm identifying patch marks with a simple regex that looks for a line starting with
@. This seems safe sincejest-diffappears to add whitespace at the start of all lines. Therefore, even a line of content that begins with@will end up padded with whitespace in the diff itself.And rather than simply stripping out the patchmarks, I'm replacing them with
-------------. Since the point of this is to avoid broken diffs, this solves that issue while still giving some indication that there are hidden lines in between those parts of the diff. Perhaps there's a better replacement indicator than that. We could certainly preserve the patch mark@@notation like@@ ------------- @@if we think that helps communicate the meaning of the new mark.