tweag / nickel

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

Comparisons for number, string, array, and record #1985

Closed jeremyschlatter closed 2 months ago

jeremyschlatter commented 3 months ago

Hello! This is my first PR to Nickel.

I was writing some Nickel configuration today and found myself wanting string comparisons. I found this open issue, and decided to take a shot at it.

Resolves https://github.com/tweag/nickel/issues/1030

jeremyschlatter commented 3 months ago

Thanks @yannham! I made the changes you suggested.

yannham commented 3 months ago

With 4th of July and unrelated stuff, the weekly was rather empty, so let's bikeshed a bit here. I think on works better in Haskell because of the infix notation compare `on` length. While on wouldn't be horrible, I have the following suggestions:

jeremyschlatter commented 3 months ago

I've been thinking about these names for an embarrassingly long amount of time and I'm starting to get semantic satiation 😅

Variants of zip and map both suggest to me that we'll be getting back a {_ : b}, not a b.

I considered at and variants of *_at but it again makes me kind of expect we'll get back a {_ : b} instead of a b.

apply doesn't have that connotation to me, so it feels better. I agree the 2-arity isn't obvious, but I'm inclined to just accept that. Besides zip the only idea I have to communicate it is appending a 2 at the end of the name, but I find that pretty ugly.

I kind of like apply_to.

That leaves my top choices as on / apply_on / apply_to.

The use case that motivated me to open this PR in the first place was sorting an array of records by name. Here's how that looks with these options:

 array.sort (record.on "name" string.compare)
 array.sort (record.apply_on "name" string.compare)
 array.sort (record.apply_to "name" string.compare)

I'm not committed to those, though, and I'm happy for you to unilaterally make a call. You have a lot more invested in nickel than I do.

yannham commented 2 months ago

Thanks for your input. I think I like apply_on, which combines the on and makes it clearer that we select one field (while to is a bit too generic IMHO and could relate to the records as well), but is more telling than just on. @jeremyschlatter would that be ok to do the renaming directly in this PR? After that we can merge.

jeremyschlatter commented 2 months ago

@yannham Done.

yannham commented 2 months ago

Great. Thanks again for contributing!