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

lib.lists.{findFirstIndex,commonPrefix}: init #243520

Merged
merged 2 commits into from
Jul 26, 2023

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Jul 14, 2023

Description of changes

Adds two new functions:

  • lib.lists.findFirstIndex :: (a -> bool) -> Any -> [a] -> (Int | Any): Find the first index in the list matching the specified predicate or return a default if no such element exists.
    lib.lists.findFirstIndex (x: x > 3) null [ 0 6 4 ]
    -> 1
  • lib.lists.commonPrefix :: [ a ] -> [ a ] -> [ a ]: The common prefix of two lists:
    lib.lists.commonPrefix [ 1 2 3 4 ] [ 1 2 4 8 ]
    -> [ 1 2 ]

This is distantly part of the path library effort, in order to be able to work with lists of path components. Relates to #243511

This work is sponsored by Antithesis

Things done
  • Docs
  • Tests

@infinisil infinisil requested a review from edolstra as a code owner July 14, 2023 17:33
@github-actions github-actions bot added the 6.topic: lib The Nixpkgs function library label Jul 14, 2023
@infinisil infinisil mentioned this pull request Jul 14, 2023
13 tasks
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jul 14, 2023
lib/lists.nix Outdated Show resolved Hide resolved
lib/lists.nix Outdated Show resolved Hide resolved
Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

I like the minimality of all this, a lot.

lib/lists.nix Outdated Show resolved Hide resolved
lib/lists.nix Outdated Show resolved Hide resolved
lib/lists.nix Outdated Show resolved Hide resolved
@tjni
Copy link
Contributor

tjni commented Jul 15, 2023

Just brainstorming here: what do you think about extracting out findFirst into findFirstIndex and reusing that here? Pseudocode being:

commonPrefixLength list1 list2 =
    let pairs = zipLists list1 list2
    let firstDiffIndex = findFirstIndex ({ fst, snd }: fst != snd) pairs
    if firstDiffIndex was found then
        firstDiffIndex
    else
        length pairs

Not sure about the performance of this or if firstFindIndex is the best generalization of the foldl' idea you have.

@tjni
Copy link
Contributor

tjni commented Jul 16, 2023

Thinking about this a little more, the following can also be a route if commonPrefixLength is otherwise not too useful by itself (which I am unsure about but think it might be only useful as a helper function for commonPrefix):

We use your algorithm and create the more general:

findFirstWithIndex = predicate: list:
    return first { index, value } matching predicate

and then use that to define:

takeWhile = predicate: list:
    let sentinel = findFirstWithIndex (elem: !(predicate elem)) list in
    if sentinel != null then
        take sentinel.index list
    else
        list

and finally:

commonPrefix = list1: list2:
    takeWhile (predicate comparing list1[i] with list2[i])

where forming that last predicate depends on some design decisions (like does predicate accept the index, do we use zipLists, do we first get the shorter list and take over it explicitly).

@roberth
Copy link
Member

roberth commented Jul 17, 2023

findFirstWithIndex

This seems like a useful function to factor out.
To simplify it a bit, you could return just the index. That way you don't have to allocate the { index, value } attrset just to ignore value. You could call it findFirstIndex then.

@infinisil infinisil force-pushed the lib.lists.commonPrefix branch 2 times, most recently from a07eb3d to 8b98e4e Compare July 19, 2023 15:00
@infinisil
Copy link
Member Author

@tjni Nice idea! This turned out to work out really well since I was able to reuse a lot of code. I removed commonPrefixLength again, it's not really necessary to expose that, it's really just a helper function as you suggested. Instead findFirstIndex is now available :)

@infinisil infinisil changed the title lib.lists.{commonPrefix,commonPrefixLength}: init lib.lists.{findFirstIndex,commonPrefix}: init Jul 19, 2023
@infinisil infinisil mentioned this pull request Jul 19, 2023
7 tasks
lib/lists.nix Outdated Show resolved Hide resolved
lib/lists.nix Outdated Show resolved Hide resolved
lib/tests/misc.nix Outdated Show resolved Hide resolved
lib/tests/misc.nix Outdated Show resolved Hide resolved
Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
@infinisil infinisil force-pushed the lib.lists.commonPrefix branch from c12fae5 to d5a90ac Compare July 19, 2023 15:35
Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

Very elegant, such compose, many wow.

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

nod

@infinisil infinisil mentioned this pull request Jul 19, 2023
2 tasks
Copy link
Contributor

@tjni tjni left a comment

Choose a reason for hiding this comment

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

Looks awesome!

@infinisil infinisil force-pushed the lib.lists.commonPrefix branch from d5a90ac to 7f61b01 Compare July 20, 2023 20:42
@infinisil infinisil merged commit bd74cdf into NixOS:master Jul 26, 2023
@infinisil infinisil deleted the lib.lists.commonPrefix branch July 26, 2023 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: lib The Nixpkgs function library 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants