Add get value from record with supplying default

[olorin37]

As I see there is lacking the function in stdlib for getting values from the record with assumed default in case field is lacking. Or maybe there is another idiomatic method for that, which I miss?

The name of the function could be discussed...Personally for me get would be the best, but it is reserved already;-> do you have better ideas?

Here also documentation should be extended...

What do you, @yannham think about it?

[olorin37]

Or maybe would be better to have dedicated syntax for that?

[yannham]

I originally wrote a comment but it's probably been lost in draft state, on the topic of having a dedicated syntax for that. I'm personally not a fan of making the syntax more complex just for a little convenience - especially in Nickel which we want to remain mostly readable to non-experts. Additionally, whether we add a new syntax or not at some point, it's still useful to have the function version, for example to pass to higher-order functions like array.map or record.map.

Beside the previous suggestions, looks good to me! The tests are currently failing because of my bogus suggestion, see the corresponding comment (it's r."%{field}", not field."%{r}"). We can merge once it's fixed.

[olorin37]

🤦 actually I haven't time to just property analyse it... Just applied your suggestions...

[olorin37]

Maybe also would be sensible to add std.array.at_or as it would be a mirror or std.record.get_or? @yannham?

[yannham]

Maybe also would be sensible to add std.array.at_or as it would be a mirror or std.record.get_or? @yannham?

Sure, it's probably not as usefulas get_or but it doesn't hurt. Given this PR is small, do you want to patch it directly @olorin37 ?

[yannham]

Suggested change

	= fun n default_value l =>
	= fun n default_value array =>

[yannham]

This primop is missing an argument.

Suggested change

	if n < %length% then
	if n < %length% array then

[olorin37]

Thanks for next review with obvious findings:) those %s misleads, and also I am used to jq.

According to names as you propose name "array" I'll do the same for "record".

[olorin37]

OK, it seems that I should update those tests snapshots, as there are different line numbers in some cases... Am I right, @yannham ?

[yannham]

Ah, yes, we have a test which checks all the snippets in the manual, including their output, by actually running the code and capturing the result. Which means some error needs to be updated when they point to inside the stdlib and the line numbers in stdlib have changed. It's a bit annoying (but those tests are still very useful to make sure we keep the examples in the manual up to date). It seems this is what's failing in the CI.

You can trigger those tests with cargo test locally, if you want to iterate faster.

[olorin37]

@yannham Hmm, I got some strange errors after adding at_or. Generally it is better to resolve such errors and understand it, I think in this case it is good opportunity to neglect it, as it is only a mirror of get_or and actually it is not as natural as get_or function.

[yannham]

I'm always fine with smaller changes 🙂 we can do at_or in a second row. Thanks for the addition!

[olorin37]

Of course, only if you feel it valuable I can do next MR with at_or:-)