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

Add stdlib function array.at_or #1927

Merged
merged 2 commits into from
May 30, 2024
Merged

Conversation

olorin37
Copy link
Contributor

This is to return value at given position of array or default in case provided position is out of bound.

Comment on lines 184 to 190
std.array.at 3 "default" [ "zero", "one" ] =>
"default"
Copy link
Member

Choose a reason for hiding this comment

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

at -> at_or + adding an example where the default value isn't chosen.

Suggested change
std.array.at 3 "default" [ "zero", "one" ] =>
"default"
std.array.at_or 3 "default" ["zero", "one", "two", "three"] =>
"three"
std.array.at_or 3 "default" ["zero", "one"] =>
"default"

core/stdlib/std.ncl Show resolved Hide resolved
core/stdlib/std.ncl Outdated Show resolved Hide resolved
This is to return value at given position of array or default in case
provided position is out of bound.

Co-authored-by: Yann Hamdaoui <yann.hamdaoui@gmail.com>
@yannham
Copy link
Member

yannham commented May 29, 2024

@olorin37 those line numbers in tests are a bit annoying. You need to run cargo insta review and accept the new version of the snapshots to make the tests pass (by the way, it's probably quicker for iteration to run cargo tests locally than to wait for the CI which isn't the fastest). If you don't have the bandwidth I can also just take over from here, if you'd prefer.

@olorin37
Copy link
Contributor Author

Yep, I already figure out staff relatet to cargo insta... But now there is a different problem... I couldn't understand it, is something what tests snippets in manual...

@olorin37
Copy link
Contributor Author

Hmm, i see that those line numbers still visible in check on CI... Locally I've already solve this ... Maybe I forgot to push last changes, as I hit next problem, but it would be strange... As I feel that I pushed those insta review changes... Maybe there is a diff between my env and CI which causes this, I'll try align CI to my env...

@yannham
Copy link
Member

yannham commented May 29, 2024

It's probably the same problem, but for manual snippets. The CI tests all manual snippets as well, including their output - for now it's not smart enough to strip unimportant information like line numbers. So changing the stdlib can always be a bit painful on that front, if you shuffle line numbers, you have to fix the corresponding excerpt which includes full error messages in the manual. Hopefully the error message should give you the diff of what line number you need to change.

Long term, it would probably be nicer to just strip stdlib line numbers from the output - we already do something similar in snapshots test to strip the local file paths from error snapshots.

@olorin37
Copy link
Contributor Author

I got following error:

> std.array.filter (fun x => if x % 2 == 0 then x else null) [1,2,3,4,5,6]

thread 'check_manual_snippets_doc_manual_typing_md' panicked at core/tests/manual/main.rs:136:49:
error: contract broken by the caller of `filter`
    ┌─ <stdlib/std.ncl>:349:25
    │
349 │       : forall a. (a -> Bool) -> Array a -> Array a
    │                         ---- expected return type of a function provided by the caller
    │
    ┌─ <repl-input-6>:1:55
    │
  1 │  std.array.filter (fun x => if x % 2 == 0 then x else null) [1,2,3,4,5,6]
    │                                                       ---- evaluated to this expression
 was expected to be a prefix of error: contract broken by the caller of `filter`
    ┌─ <stdlib/std.ncl>:373:25
    │
373 │       : forall a. (a -> Bool) -> Array a -> Array a
    │                         ---- expected return type of a function provided by the caller
    │
    ┌─ <repl-input-6>:1:55
    │
  1 │  std.array.filter (fun x => if x % 2 == 0 then x else null) [1,2,3,4,5,6]
    │                                                       ---- evaluated to this expression

note:
  ┌─ <repl-input-6>:1:2
  │
1 │  std.array.filter (fun x => if x % 2 == 0 then x else null) [1,2,3,4,5,6]
  │  ------------------------------------------------------------------------ (1) calling filter


note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    check_manual_snippets_doc_manual_typing_md

test result: FAILED. 9 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 3.31s

@olorin37
Copy link
Contributor Author

@yannham OK, so now on CI you can see same problem which I have. I do not know why filter function somewhere is broken now?

@vkleen
Copy link
Contributor

vkleen commented May 29, 2024

The filter is a red herring. The problem is that the line number in std.ncl where filter is defined has changed, and the test checks the output of Nickel evaluations in the manual verbatim. In particular, it checks the error messages and the line number appears in them.

@olorin37
Copy link
Contributor Author

Nice, OK. So, how to change those line numbers? Because cargo insta review doesn't ask for accepting filter snippe?

@vkleen
Copy link
Contributor

vkleen commented May 29, 2024

For the manual I think you need to change them manually, they don't go via cargo insta. I.e. just edit doc/manual/typing.md (in this case).

@olorin37
Copy link
Contributor Author

olorin37 commented May 29, 2024

Ok, Ill try:)

@olorin37
Copy link
Contributor Author

OK, so now you can freely review and approve it it is OK. @yannham or @vkleen. Thanks for guidance!

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.

Looks great, thanks!

@yannham yannham added this pull request to the merge queue May 30, 2024
Merged via the queue into tweag:master with commit c69a48b May 30, 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.

3 participants