search

tweag / nickel

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

Add get value from record with supplying default #1920

Closed olorin37 closed 1 month ago

olorin37 commented 1 month ago

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

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

olorin37 commented 1 month ago

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

olorin37 commented 1 month ago

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

yannham commented 1 month ago

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 ?

olorin37 commented 1 month ago

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

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

yannham commented 1 month ago

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

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

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

olorin37 commented 1 month ago

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