tweag / nickel

Better configuration for less
https://nickel-lang.org/
MIT License
2.23k stars 85 forks source link

Add stdlib function array.at_or #1927

Closed olorin37 closed 1 month ago

olorin37 commented 1 month ago

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

yannham commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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

vkleen commented 1 month ago

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 commented 1 month ago

Ok, Ill try:)

olorin37 commented 1 month ago

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