nickel icon indicating copy to clipboard operation
nickel copied to clipboard

Add get value from record with supplying default

Open olorin37 opened this issue 1 year ago • 1 comments

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 avatar May 17 '24 22:05 olorin37

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

olorin37 avatar May 19 '24 07:05 olorin37

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

olorin37 avatar May 21 '24 07:05 olorin37

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

olorin37 avatar May 21 '24 11:05 olorin37

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 avatar May 21 '24 12:05 yannham

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 avatar May 21 '24 16:05 olorin37

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

olorin37 avatar May 21 '24 18:05 olorin37

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.

yannham avatar May 21 '24 20:05 yannham

@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.

olorin37 avatar May 23 '24 23:05 olorin37

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

yannham avatar May 24 '24 22:05 yannham

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

olorin37 avatar May 25 '24 00:05 olorin37