Skip to content

Conversation

@Hazarean
Copy link

🚀 Pull Request

Description

Refactors the test_coord_equality function 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.

@CLAassistant
Copy link

CLAassistant commented Dec 17, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@trexfeathers trexfeathers left a 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 A good issue to take on if you're just getting started with Iris development !

Copy link
Contributor

@trexfeathers trexfeathers left a 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 😂

@Hazarean
Copy link
Author

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.

@ESadek-MO
Copy link
Contributor

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!

@Hazarean
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants