Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
Jul 8, 2024

Conversation

jeremyschlatter
Copy link
Contributor

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 #1030

Copy link
Member

@yannham yannham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for taking this on! It looks pretty good, beside a few secondary comments.

core/stdlib/std.ncl Outdated Show resolved Hide resolved
core/stdlib/std.ncl Outdated Show resolved Hide resolved
@@ -2761,6 +2810,23 @@
|> std.array.filter (fun { field, value } => f field value)
|> from_array,

on
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the name of this function is not very descriptive of what it does. I don't have a great inspiration right now, and it is a bit hard to describe as it's pretty abstract. We'll bikeshed that in the weekly meeting tomorrow and I'll report back here.

Copy link
Contributor Author

@jeremyschlatter jeremyschlatter Jul 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's not obvious to me what to call this. For what it's worth, Haskell has a function on that is very similar:

(b -> b -> c) -> (a -> b) -> a -> a -> c

The Nickel function here is the same as Haskell's on except:

  • the arguments are flipped
  • a is specialized to { _ : b }
  • (a -> b) is specialized to accessing a named field

Curious to hear how the bikeshedding goes.

core/stdlib/std.ncl Outdated Show resolved Hide resolved
use std::cmp::Ordering;
Ok(Closure::atomic_closure(RichTerm::new(
Term::Enum(LocIdent::new_with_pos(
match s1.cmp(s2) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first I thought we would need to do something unicode-aware, because Nickel uses everywhere grapheme clusters as its unit of string characters. However, the Rust comparison is just comparing byte by byte lexicographically. I guess doing that, or first splitting by grapheme clusters, and then comparing cluster by cluster (using the normal string comparison) is entirely equivalent, so this is fine.

@jeremyschlatter
Copy link
Contributor Author

Thanks @yannham! I made the changes you suggested.

@yannham
Copy link
Member

yannham commented Jul 5, 2024

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:

  • apply_on/map_on/get_map. I think those are more telling that we map a function over a specific field. However, it's not clear that the function has arity 2 (get_map sounds more like forall a b. String -> (a -> b) -> {_ : a} -> b). We can add 2 as a suffix but it's less nice
  • zip_on: there is the precedent of zip_with, so zip is already used for applying a binary function with arguments coming from two different sources, which is nice. Maybe it should be theoretically zip_on_with but it's slightly uglier IMHO.

@jeremyschlatter
Copy link
Contributor Author

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
Copy link
Member

yannham commented Jul 8, 2024

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
Copy link
Contributor Author

@yannham Done.

@yannham
Copy link
Member

yannham commented Jul 8, 2024

Great. Thanks again for contributing!

@yannham yannham added this pull request to the merge queue Jul 8, 2024
Merged via the queue into tweag:master with commit 58ef79d Jul 8, 2024
4 checks passed
@jeremyschlatter jeremyschlatter deleted the comparison-issue-1030 branch July 8, 2024 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add comparison combinators for sorting to the standard library
2 participants