-
Notifications
You must be signed in to change notification settings - Fork 300
Refactor non-idiomatic assertions in test_coord_equality #6862
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?
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.
Thanks very much for the contribution @Hazarean!
Bad news though: I've just been investigating and it turns out we were wrong to flag this as a bad practice (#6625) in the first place. Tests like this are necessary because the objects being tested implement methods such as:
__ne__:!=__gt__:>__le__:<=
... so those operators need to be explicitly tested, even though it ends up looking wrong. I've modified #6625 accordingly. I've learned something today, so thank you for that.
We would welcome another proposal if you're up for it. Plenty left under
Good First Issue
trexfeathers
left a comment
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.
Actually, on reflection, would you mind adding a comment to the code explaining that this isn't a mistake? Given this has caught out several of the team 😂
Thanks for the quick response and explanation! I'm not able to make the update today but I'll add the explanatory comment as suggested and push an update shortly. |
|
Hi @Hazarean, Just to double check, the commit message suggests you intended to add the clarifying comment, but it seems only the reversion of previous changes were pushed! |
|
Hi @ESadek-MO, My bad! I'm still getting to grips with the Git workflow. I've just pushed a follow-up commit that includes the note. |
🚀 Pull Request
Description
Refactors the
test_coord_equalityfunction to replace non-idiomatic assertions (using 'not') with idiomatic equivalents.No functional behaviour is changed; the change improves readability and maintainability of the test code.
Related to issue #6625.