-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
NotSame should fail if args are not pointers #1661 #1664
Conversation
Hi, @brackendawson. |
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.
Sorry for the late re-review, that's looking good, just a couple more bits.
assert/assertions.go
Outdated
expected, expected), msgAndArgs...) | ||
same, ok := samePointers(expected, actual) | ||
|
||
if ok { |
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.
Almost there, if you flip this to if !ok {
then you can avoid the nested if statements and fully preserve line of sight.
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 don't really want the blank line first as this is "error handling" for the line above.
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've made the changes
name string | ||
args args | ||
same BoolAssertionFunc | ||
ok BoolAssertionFunc |
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.
That's perfect. 👌
Can you add a couple more test cases for when the left and right arguments aren't pointers but the other one is, like:
args{first: 1, second: p}
args{first: p, second: 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.
Yup, I've added 2 additional test cases now
…nters in assert/assertions_test.go
… in assert/assertions.go
Hi @brackendawson. I've made the requested changes. Let me know if any other changes need to be made. |
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 is spot on 👍, just found one doc-string inaccuracy.
Co-authored-by: Bracken <abdawson@gmail.com>
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, thank you for your contribution.
Thanks for the opportunity :) |
Summary
Reduces the confusion assosciated with NotSame that previously would check nothing if any of the values is not a pointer. The changes made were tested using TestSame, TestNotSame, and Test_samePointers tests, and the changes did clear the tests.
Changes
Motivation
Ensure NotSame accurately verifies pointer sameness by handling non-pointer inputs explicitly, improving clarity and reducing potential misuse.
Related issues
Closes #1661