@amtoine I think with this change even using assert to check equality is more helpful than assert eq, see the error messages here:

With assert

assert (1 == 1)
assert (10 == 20)
assert (200 == 200)

Error:
  × Assertion failed: No message
    ╭─[myscript:10:1]
 10 │ assert (1 == 1)
 11 │ assert (10 == 20)
    ·         ────┬───
    ·             ╰── It is not true.
 12 │ assert (200 == 200)
    ╰────

With assert eq

assert eq 1 1
assert eq 10 20
assert eq 200 200


Error:
  × left is not equal to right
   ╭─[myscript:5:1]
 5 │     if not $cond {
 6 │         error make {msg: $msg}
   ·         ─────┬────
   ·              ╰── originates from here
 7 │     }
   ╰────

I'd consider the value added by assert eq.

At first step I did not want to make much change, just send the proposed assert command. But if you like it, I can follow up.

Codecov Report

Merging #8405 (3ceb74b) into main (a52386e) will increase coverage by 0.40%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #8405      +/-   ##
==========================================
+ Coverage   68.11%   68.51%   +0.40%     
==========================================
  Files         620      620              
  Lines       99709    99723      +14     
==========================================
+ Hits        67914    68329     +415     
+ Misses      31795    31394     -401     

@presidento
thanks a lot for this, it's already something that came up in #8368 and i do think having output on the std assert familly of commands is a must-have 🥳

i'll have a look after lunch for sure 😉

Here is the next step. The _assert cannot be used, since making errors suppose that it was directly called from user code. (If assert ne calls assert, it will just show that $left and $right are not equal.) I extracted the error making step, the only thing which can be common.

I implemented it for assert eq, although I think it would be better to use full name, not abbreviation: assert equal. For example Nushell uses str substring, not str substr. Also I think it would better to use assert not equal instead of assert ne.

I am confused with assert ne:

>_ 1 != "2"
true

>_ assert ne 1 "2"
Error:
  × left and right operand of `assert eq` should have the same type
   ╭─[myscript.nu:5:1]
 5 │     if not $cond {
 6 │         error make {msg: $msg}
   ·         ─────┬────
   ·              ╰── originates from here
 7 │     }
   ╰────

@presidento
no need to rebase and force push, it's normal to apply changes as reviews come to a PR 😉

and that's not the most convenient for reviewers to checkout your branch and test what has been done, i think 😋

i think the names have been chosen to reflect what is done with rust, i.e. assert, assert_eq and assert_ne 👍
maybe that should be for another PR?

and about assert ne it is simply the exact same thing as assert eq but with the boolean equality check being reversed, i.e. == becomes a != 😉

In my organization we are allowed to push only perfect commits to master, so we rebase&change commits as long as they are not in the preferred state. If it has no value for this repository, I can leave existing commits as they are and do every change in follow-up commits.

In my organization we are allowed to push only perfect commits to master, so we rebase&change commits as long as they are not in the preferred state. If it has no value for this repository, I can leave existing commits as they are and do every change in follow-up commits.

the nushell merge strategy is to squash all the commits and rebase the commit onto the main branch.
the effect is that intermediate commits never end up in the main branch, but we can still reopen a PR branch and see a particular commit with a bug introduced for instance 😊

not sure what to do with these failing tests 🤔

i think assert eq and assert ne should raise an error when the types of the left and right operands are different but more importantly, with or without this type check, they shoudld definitely be consistent 😋

Note
maybe we need more feedback on this assert ne
should assert ne not check the type because things like 1 != "2" work?
in that case, the comparison looks ill defined to me because 1 and "2" do not even have the same type, so yeah it is true that they are not equal, but to me they look not even comparable 😮

[edit: just reread your question above @amtoine, you're right. So read my original response as support for removing the type check from the asserts...]

It appears that Nushell will let you compare items of different types without complaining about type mismatch.
Therefore, I don't think assert eq or assert ne should insist that both sides be of the same type.

〉1 == 1.0
true
〉true == 1
false

The 1 == 1.0 case isn't a crystal clear example of this, but surely true is boolean and 1 is not.

Due to having 2 devs busy in the same 2 files on the same day, this PR drops out the automatic loading of dirs.nu from std.nu and the unit tests for dirs that were added to tests.nu earlier today. Please rebase on the very latest main to pick up those changes.

This is developer-hostile design, we shouldn't have such race conditions for unrelated sub-modules. Ideally, developer of a sub-module (like dirs.nu) could simply drop a dirs.nu and a test_dirs.nu somewhere under standard_library/ and the module would be automatically loaded and the unit tests automatically run.

@fdncred @kubouch

i think we need some help here to decide what to do with assert eq and assert ne 😮

TL;DR: should assert eq 1 "1" and assert ne 1 "2" give errors or something else?
and as added by @presidento

Please include assert eq 1 1.0 to this topic as well. (At current implementation it gives error, while 1 == 1.0 is true.)

@presidento

i can say for sure @bobhy and i are not members of the core team 👍

the standard library is a very recent project with a lot of design being defined right now 😮
i hope did not induce too much confusion about who was "in charge" and what had to be done here...

I'd defer to @kubouch's opinion.

(I reverted the change of behaviour and created additional issues for the mentioned topics. Now this PR is just about the detailed error messages and be able to add custom message to assert methods.)

By reading the posts here, it seems like it should work before except Now this PR is just about the detailed error messages and be able to add custom message to assert methods. If that's not the case, then someone should investigate to try and understand what changed.

There should be no behaviour change at all. Let me explain it. (Also, there are unit tests for these commands.)

assert

It is the same. Previously if not $cond: error, now if not $cond return; else error

assert eq

Previously if not (types are equal): error then if not ($left == $right): error.
Now if $type_left != $type_right: error then if $left != $right: error.

assert ne

Previously if types are equal: error then if not ($left != $right): error.
Now if $left == $right: error then if $type_left != $type_right: error.

Here the order changed to have a better error message, but still

• it passes if both assertion pass
• makes an error if one of them makes an error

Custom message for assertions

Yes, it is changed, now you can add custom messages, but it is optional, so no behaviour change for existing codes:

assert (1 == 2) "It should fail"
assert eq 1 2 "It should fail"
assert ne 1 2 "It should pass"

assert

👍

assert eq

👍

assert ne

Previously if types are equal: error then if not ($left != $right): error.
Now if $left == $right: error then if $type_left != $type_right: error.

this is where i disagree 😋

previously we had

assert ne [l, r] {
    assert (l.type == r.type)  # error when `l.type != r.type`
    assert (l.val != r.val)
}

but now we have

assert ne [l, r] {
    if (l.val == r.val): error
    if (l.type != r.type): error
}

which does not look the same to me 😮

• maybe the definitions of == and != in nushell do not show the non equivalence
• but as the type check is not after the value check, the two are clearly not equivalent

i hope i'm not becoming blind to what is written here, i've seen so much asserts these days 😆

i'm sorry being that picky here...
i'm just really confused about the logical inversion introduced by this PR 🤔

before we had the _assert wrapper that, at least to me, made the checks in assert, assert eq and assert ne quite clear.
now all the boolean tests have been inverted and even some of the orders between the tests themselves, to accomodate to the new _assertion-error.

At the end of the day it is your code. If you don't like the style, feel free to "be inspired" with this PR, reject this one and resend a new one with the style you want. I will 👍 it. From functional point of view I am sure they are identical.

At the end of the day it is your code.

nope it is absolutely not, i know "my" asserts had a lot of flaws that need to be addressed 👍

From functional point of view I am sure they are identical.

i did not check all the workings of == and != in nushell, so i trust you on this 😌

If you don't like the style, feel free to "be inspired" with this PR, reject this one and resend a new one with the style you want. I will +1 it.

what i'm saying is that the whole logic inversion introduced here is confusing to me, merely to add better message that just require passing the span of the condition being tested 🤔
i might be the only one confused, but still, i was 😉

in the end, "changing the style" might have been better kept in a separate and clearer PR, leaving the "better messages" (and they truely are, don't get me wrong 💪) for this one.

@fdncred
i think we're good 😉

For having better error messages, the error-making part must be extracted, since it is the (only) common part. It has an inverse behaviour compared to _assert. So keeping the code as it was while extracting the inverse behaviour, well, can be done, but it would made the code more complex. I rewrote the assert ne command to use exactly the same checks as the original one had. I hope it makes clear that there were no behaviour changes here.

Other than not having no-op blocks, this code seems pretty good to me.

Done

Let's land this then :)

🥳 noishe