Skip to content

stdlib: Implement common assert commands#8515

Merged
fdncred merged 4 commits intonushell:mainfrom
presidento:asserts
Mar 20, 2023
Merged

stdlib: Implement common assert commands#8515
fdncred merged 4 commits intonushell:mainfrom
presidento:asserts

Conversation

@presidento
Copy link
Copy Markdown
Contributor

Implement common assert commands with tests.
Fixes #8419.

@presidento
Copy link
Copy Markdown
Contributor Author

After this and #8499 I would like to write a chapter about testing in Nushell in the book.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 18, 2023

Codecov Report

Merging #8515 (8c33be8) into main (ecc153c) will decrease coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8515      +/-   ##
==========================================
- Coverage   68.51%   68.51%   -0.01%     
==========================================
  Files         624      624              
  Lines      100562   100562              
==========================================
- Hits        68903    68902       -1     
- Misses      31659    31660       +1     

see 1 file with indirect coverage changes

Copy link
Copy Markdown
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i like the fact that you built everything around the assert command 👍

and the assert error is 👌

only minor things i saw and i'll approve this 😉

# > assert less or equal 1 2 # passes
# > assert less or equal 1 1 # passes
# > assert less or equal 1 0 # fails
export def "assert less or equal" [left: any, right: any, message?: string] {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for less or equal, less, greater and greater or equal, maybe you should annotate the left and right arguments with number instead of any.
it looks we can only compare integers, floats, hexadecimal values, ...
these can all be annotated with the number type 👍

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Numbers are not comparable:

  × Types mismatched for operation.
     ╭─[std.nu:112:1]
 112 │ export def "assert less or equal" [left: number, right: number, message?: string] {
 113 │     assert ($left <= $right) $message --error-label {
     ·             ──┬── ─┬ ───┬──
     ·               │    │    ╰── number
     ·               │    ╰── doesn't support these values.
     ·               ╰── number
 114 │         start: (metadata $left).span.start
     ╰────
  help: Change number or number to be the right types and try again.

@presidento
Copy link
Copy Markdown
Contributor Author

Thank you for the detailed review. I like your comments, I will do them on Monday.

@sholderbach sholderbach added the A:std-library Defining and improving the standard library written in Nu label Mar 18, 2023
We NEVER use == to compare floats.
Except in Nushell?
@presidento
Copy link
Copy Markdown
Contributor Author

It is ready to be approved. It seems to fail for an unrelated thing.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Mar 20, 2023

I think you'll need to merge main to get rid of this ci error.

@presidento
Copy link
Copy Markdown
Contributor Author

I merged main, now it is green.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Mar 20, 2023

Thanks

@fdncred fdncred merged commit 7d96377 into nushell:main Mar 20, 2023
@presidento presidento deleted the asserts branch April 6, 2023 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A:std-library Defining and improving the standard library written in Nu

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Naming conventions for assert commands

4 participants