-
Notifications
You must be signed in to change notification settings - Fork 31k
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
util: expose diff function used by the assertion errors #57462
util: expose diff function used by the assertion errors #57462
Conversation
1905f00
to
480d3c7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #57462 +/- ##
==========================================
+ Coverage 90.20% 90.23% +0.02%
==========================================
Files 629 630 +1
Lines 184948 184898 -50
Branches 36204 36217 +13
==========================================
- Hits 166837 166834 -3
+ Misses 11057 11019 -38
+ Partials 7054 7045 -9
🚀 New features to boost your workflow:
|
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.
Code change LGTM with one nit about the docs.
480d3c7
to
518712d
Compare
The
notable-change
Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section. |
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 (only one nit)
549a8b6
to
1a33a1c
Compare
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.
This looks pretty good!
I would still consider changing the output format to be the most performant one we can think of. I would likely change it to: [type, value]
, where type
would be one of -1, 0, 1
.
f4b06e7
to
fe1f36c
Compare
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.
Nice! This is shaping up well!
Two small things, otherwise it's LGTM
fe1f36c
to
dd09392
Compare
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.
Great work
Landed in 6b42554 |
fix: #51740
Exporting the new
util.diff(actual, expected)
function.This function is the one internally used by the
assert
module when anAssertionError
occurs and it is just an utility to better show differences between the two paramsactual
andexpected
.It is internally using the "new"
Myers
diff algorithm so you can expect the same output provided by this PR: #54862