Skip to content
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

Merged
merged 1 commit into from
May 24, 2024

Conversation

ajbt200128
Copy link
Contributor

@ajbt200128 ajbt200128 commented May 11, 2024

Previously Nickel was using an Assert only for integration tests. This pr makes it officially part of the standard library.

Closes #1300

@ajbt200128
Copy link
Contributor Author

Note the original issue recommends adding these to std.test . I can move Assert and check there, and maybe Equal belongs there too. Added it to std.contract for now since Equal is there already

Copy link
Member

@yannham yannham left a 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.

HACKING.md Outdated Show resolved Hide resolved
core/stdlib/std.ncl Outdated Show resolved Hide resolved
core/stdlib/std.ncl Outdated Show resolved Hide resolved
core/stdlib/std.ncl Outdated Show resolved Hide resolved
core/tests/integration/inputs/contracts/contracts.ncl Outdated Show resolved Hide resolved
@ajbt200128
Copy link
Contributor Author

Looks like pre-commit is failing, must not have run properly on my machine

@ajbt200128 ajbt200128 requested a review from yannham May 14, 2024 07:05
Copy link
Member

@yannham yannham left a 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!

core/stdlib/std.ncl Outdated Show resolved Hide resolved
core/stdlib/std.ncl Outdated Show resolved Hide resolved
@yannham yannham force-pushed the austin/assert-stdlib branch 2 times, most recently from 7eaacc5 to 84b0996 Compare May 17, 2024 08:31
@yannham
Copy link
Member

yannham commented May 17, 2024

After some discussions, we chose to put it in a new dedicated module std.test.

@yannham yannham force-pushed the austin/assert-stdlib branch 2 times, most recently from 4f4c6bd to f700849 Compare May 20, 2024 08:36
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>
@yannham
Copy link
Member

yannham commented May 24, 2024

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.

@yannham yannham added this pull request to the merge queue May 24, 2024
Merged via the queue into tweag:master with commit 928cf14 May 24, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add testing infrastructure to the standard library
2 participants