image-rs / imageproc

Image processing operations
MIT License
736 stars 144 forks source link

Rename arguments of `assert_pixels_eq` #605

Closed cospectrum closed 3 months ago

cospectrum commented 3 months ago

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 commented 3 months ago

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

cospectrum commented 3 months ago

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

ripytide commented 3 months ago

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

cospectrum commented 3 months ago

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 commented 3 months ago

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

theotherphil commented 3 months ago

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.