-
Notifications
You must be signed in to change notification settings - Fork 90
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
chore: add assert to the stdlib #1914
Conversation
Note the original issue recommends adding these to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! I think it's good to have them in the stdlib indeed, as those functions might be useful to others. Though, as I commented, this might mandate a renaming.
Regarding where to put them, this is a good question. I don't think they belong to std.contract
, which is mostly about contract combinators and operations. It's true that there are also contract definitions, like Equal
, but because they wouldn't make sense anywhere else. For example, topic contracts like PosNat
or TagOrString
are in their respective topic module (std.number
, std.enum
).
One solution is to think that those functions are so fundamental that they should be at the top-level std
, like std.trace
. However we probably want to avoid putting to many things here. Another solution is to have a std.assert
or std.test
. I would lean toward this solution, but let's discuss this issue in the next weekly meeting.
In the meantime, we can move forward with this PR; it's easy to move those functions around (and/or rename them) after the fact, before the next release. Mostly the documentation of those functions need a tiny bit of massaging to make them more "general" and stdlib-ready.
Looks like pre-commit is failing, must not have run properly on my machine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beside a few comments on the doc, looks good!
7eaacc5
to
84b0996
Compare
After some discussions, we chose to put it in a new dedicated module |
4f4c6bd
to
f700849
Compare
Move the Assert and check function from the local test library to the stdlib, under the name of `std.test.Assert` and `std.test.assert_all`. Co-authored-by: Yann Hamdaoui <yann.hamdaoui@gmail.com>
f700849
to
d46d53a
Compare
So, this PR was delayed for reasons that have nothing to do with the original change (mostly related to our flake being outdated because of unrelated C++ compilations errors for the nix-experimental feature), but I think I can finally proceed now. |
Previously Nickel was using an Assert only for integration tests. This pr makes it officially part of the standard library.
Closes #1300