Skip to content

Introduce new lib-function for lists to filter out empty elements#2566

Closed
SebTM wants to merge 1 commit intonix-community:masterfrom
SebTM:lib_lists_notEmpty
Closed

Introduce new lib-function for lists to filter out empty elements#2566
SebTM wants to merge 1 commit intonix-community:masterfrom
SebTM:lib_lists_notEmpty

Conversation

@SebTM
Copy link
Copy Markdown
Collaborator

@SebTM SebTM commented Dec 15, 2021

Description

Related to #2314
Moved function needed by this PR because its providing basic/general functionality and therefore should be located in lib

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

    • Added myself and the module files to .github/CODEOWNERS.

@sumnerevans sumnerevans requested a review from berbiche December 16, 2021 01:56
@rycee
Copy link
Copy Markdown
Member

rycee commented Dec 26, 2021

I think it's best to avoid introducing this function, I think it is too ambiguous as to what "empty" means in this context. For example, why not remove empty lists as well? And the flatten functionality seems arbitrary. So it seems to me that this function is not actually generally useful but rather intended for a specific use-case.

I'm thinking that perhaps a function like

removeAny [3 4] [ 1 3 4 3 ]
# => [ 1 ]

to match the remove function may be nice. The implementation would be something like

removeAny = l: builtins.filter (x: ! builtins.elem x l)

It wouldn't do any flattening, though, but I think that is an orthogonal functionality.

@SebTM
Copy link
Copy Markdown
Collaborator Author

SebTM commented Dec 27, 2021

Hmm, the function/separation outside the module happened after discussion in the above PR, as the "notEmpty"-function comes from "xidlehook" it felt like a more often use-case with the below examples:

The goal with providing this function was to reduce the possible duplicate code and more common handling of these recurring situations, as far as I understand.

I understand your point with the naming, we can rename and extend it to also filter empty lists 👍🏻
I'm not sure how to understand "And the flatten functionality seems arbitrary" can you explain a bit more, please?

Thanks for your review and feedback, I appreciate it ⭐

rycee added a commit that referenced this pull request Dec 27, 2021
@rycee rycee mentioned this pull request Dec 27, 2021
5 tasks
@rycee
Copy link
Copy Markdown
Member

rycee commented Dec 27, 2021

@SebTM About the flatten, I think that the flattening is so different from the notEmpty naming that it would have to be included in the name, similar to how concat is part of concatMap. Specifically, with the existing naming I would expect

notEmpty [ [ "" 34 ] [] null "s" ]

to return [ [ "" 34 ] "s" ], not [ 34 "s" ]. So perhaps something like flattenRemoveEmpty and removeEmpty (for the non-flattening version of the function).

I'm not super convinced about the necessity of these functions, though. When it only is empty strings that are removed then I think remove "" is more straight-forward and clear. I created the #2590 PR to make that change. The filter remains for cases where you want to remove, for example, both null and "" values. For those perhaps the removeAny (alternatively named removes) function would be simplest. I.e., something like

removes [ "" null ] listOfStuff

The flattening could be done explicitly

removes [ "" null ] (flatten listOfStuff)

or with a custom function

flattenRemoves [ "" null ] listOfStuff

@SebTM SebTM closed this Mar 2, 2022
@SebTM
Copy link
Copy Markdown
Collaborator Author

SebTM commented Mar 2, 2022

Seems to be not really needed as I could use lists.subtractLists without Problems - see #2314 :)

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.

3 participants