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

Add array.zip_with and array.imap to the standard library #1797

Merged
merged 5 commits into from
Feb 1, 2024

Conversation

vkleen
Copy link
Contributor

@vkleen vkleen commented Feb 1, 2024

This change adds the functions zip_with andimap to the array standard library. Both were quite useful in a recent project of mine and seem general enough to warrant inclusion.

@vkleen vkleen requested a review from yannham February 1, 2024 13:25
@github-actions github-actions bot temporarily deployed to pull request February 1, 2024 13:29 Inactive
@github-actions github-actions bot temporarily deployed to pull request February 1, 2024 13:51 Inactive
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.

This is a fair addition. I wonder about the naming though. OCaml and Scala use map2 instead of zip_with (and mapi instead of imap). Rust doesn't have them, I guess you use .enumerate() or .zip() and then do a normal map. In the JavaScript world, lodash (some kind of common utility extension of the stdlib) use zipWith.

While I understand why zip_with makes sense in a language with collections, tuples, curryfication and higher order functions, I wonder if the convention of all those variants being just map with a different suffix isn't more discoverable. But maybe that's just the stubborn Ocamler inside me 😕

core/stdlib/std.ncl Outdated Show resolved Hide resolved
Co-authored-by: Yann Hamdaoui <yann.hamdaoui@tweag.io>
@github-actions github-actions bot temporarily deployed to pull request February 1, 2024 15:13 Inactive
@vkleen
Copy link
Contributor Author

vkleen commented Feb 1, 2024

The biggest reason to go with zip_with alone and not with zip followed by map is that Nickel doesn't have first-class tuples. Both zip_with and imap as names betray my Haskell background, I guess. I wouldn't be opposed to map2 (and eventually map3 and so on), though.

On a side note, an error message in the manual currently depends on the line count of std.ncl, which scares the CI. I wonder if there's an easy way to hide that.

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.

Let's discuss that further in the weekly meeting, and merge the implementation as it is. We just need to decide before the next version.

@yannham
Copy link
Member

yannham commented Feb 1, 2024

On a side note, an error message in the manual currently depends on the line count of std.ncl, which scares the CI. I wonder if there's an easy way to hide that.

Right, I had the same issue on a recent PR. Maybe we can use the same trick that we use for snapshot testing, where we strip off the local paths and replace them with a generic placeholder (or, rather, a co-placeholder 🤔 - a placetaker ?)

@github-actions github-actions bot temporarily deployed to pull request February 1, 2024 15:32 Inactive
@vkleen vkleen added this pull request to the merge queue Feb 1, 2024
Merged via the queue into master with commit 201741a Feb 1, 2024
5 checks passed
@vkleen vkleen deleted the std-array-additions branch February 1, 2024 15:49
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.

2 participants