trycmd
trycmd copied to clipboard
assert-rs
Snapbox parameter order (expected, actual) breaks common convention
The following code:
snapbox::assert_eq("foo", "bar");
results in:
--- Expected
+++ Actual
1 - foo∅
1 + bar∅
which funnily enough is rather unexpected. When writing comparisons in code it is common practice to put constants on the right side of the operator e.g:
if self.length == 0 {
rather than:
if 0 == self.length {
And even though std::assert_eq does not document any convention and also just speaks of left and right in its panic messages, the vast majority of Rust code uses the same convention of putting the expected value last (e.g. assert_eq!(self.length, 0) instead of assert_eq!(0, self.length).
The non-file-based methods of snapbox however use the opposite order ... which matters because snapbox (contrary to std, pretty_assertions and similar-asserts does label its diff with expected/actual instead of just left/right).
Note that I think attempting to label actual/expected for non-file based methods is a good idea ... it's just that I think not breaking the user expectation is more important than anything else. We could still achieve both labeling and adhering to user expectations but it would require a breaking change.
For std::assert_eq, the order would only have minor rendering differences. For snapbox, the order makes all the difference. I've seen some frameworks treat it the other way around (likely coming from code bases using yoda speak). Personally, I normally prefer assert_eq!(actual, expected) . There was a specific reason I found that here it worked better and it'd take some digging through history to try to uncover the reason but snapbox isn't a focus area of mine atm (I rotate my focus between projects to reduce context switching). I wouldn't want to evaluate a change without that original background.
I've never seen frameworks using (expected, actual) but I don't doubt that they exist. Haha ... this is also the first time for me hearing about "yoda conditions" ... Wikipedia even has an article about them.
I wouldn't want to evaluate a change without that original background.
That is very understandable.
Since snapbox using (actual, expected) is somewhat unexpected, I think this would deserve a section in the root module doc comment. If you expect that you're not getting to address this issue in the near future (which is also very understandable), I'd be happy to make a PR to add such a documentation section. Let me know if you'd like such a PR.
Yes for a function called "diff" I'd agree that (old, new) makes much more sense than (new, old). However the function is called "assert" and as I explained the common convention for assert statements is putting actual before expected.
as I explained the common convention for assert statements is putting actual before expected
Out of curiosity, are there any Rust libraries that do this?
I mean, although this might be the correct way, the breaking change is not compelling to make, imagine you wake up tomorrow and your function is inverted.
#296 was pulled out of main into a branch for now and looking to re-apply this which led me down a rabbit hole.
The first is what the signature for
pub fn try_eq(
&self,
expected: crate::Data,
actual: crate::Data,
actual_name: Option<&dyn std::fmt::Display>,
) -> Result<()> {
should be
likely
pub fn try_eq(
&self,
actual_name: Option<&dyn std::fmt::Display>,
actual: crate::Data,
expected: crate::Data,
) -> Result<()> {
Then in talking over the above with @Muscraft that led to "why is there a preference for one over the other" and this got me looking at prior art
- rust only has
(left, right) - pretty_assertions maintains the concept of
(left, right)and lines up with(expected, actual)for how we show the diff- same for similar-asserts
- insta inline snapshots do
(actual, snapshot)