Skip to content
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

Rename arguments of assert_pixels_eq #605

Closed
cospectrum opened this issue May 14, 2024 · 6 comments · Fixed by #614
Closed

Rename arguments of assert_pixels_eq #605

cospectrum opened this issue May 14, 2024 · 6 comments · Fixed by #614

Comments

@cospectrum
Copy link
Contributor

Currently, the first argument is named as actual and the second as expected. But even in imageproc tests, someone has already messed up the order. For me, the standard output was very unexpected.
Therefore, I suggest simply renaming the arguments to left and right, similar to rust stdlib.

@ripytide
Copy link
Contributor

If that order was unexpected why not just fix that probably accidental mistake?

@cospectrum
Copy link
Contributor Author

cospectrum commented May 14, 2024

This is a common mistake, everyone will make it at some point. We can fix this once and for all.

@ripytide
Copy link
Contributor

Isn't it still useful to have a convention of argument order though? It makes reading the source code easier.

@cospectrum
Copy link
Contributor Author

But you don't need a biased standard output for that, name a variable.
And there are situations when there is no actual or expected result, you just want to compare.

@ripytide
Copy link
Contributor

Fair enough, I would still suggest fixing the wrongly ordered assert though for consistencies sake.

@theotherphil
Copy link
Contributor

I prefer to have a consistent ordering of actual vs expected values in test definitions and error messages. I’ll leave this issue open until I’ve checked for other tests with back to front arguments in their assertions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants