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

filteredHas combinator #806

Open
yairchu opened this Issue Jun 5, 2018 · 12 comments

Comments

Projects
None yet
4 participants
@yairchu
Contributor

yairchu commented Jun 5, 2018

I'm suggesting this combinator:

filteredHas :: Lens.Fold s i -> Lens.IndexedTraversal' i s s
filteredHas fold f val =
    case val ^? fold of
    Nothing -> pure val
    Just proof -> Lens.indexed f proof val

It can replace code like this:

Lens.filtered (Lens.has part) %~ \x -> something (x ^?! part) x

With filteredHas part %@~ something. I think it's quite useful and am using it on an expression tree sugaring pass.

Disclaimer: It's quite possible it already exists and I just haven't found it, or has a very succinct way to write it that makes it unnecessary. I often have difficulty finding combinators in lens.

@yairchu yairchu changed the title from `filteredHas` combinator to filteredHas combinator Jun 5, 2018

@ekmett

This comment has been minimized.

Show comment
Hide comment
@ekmett

ekmett Jun 5, 2018

Owner

It doesn’t really pass the Fairbairn threshold for me, but it’s come up enough that I’m open to it.

Maybe filteredBy would read better?

Owner

ekmett commented Jun 5, 2018

It doesn’t really pass the Fairbairn threshold for me, but it’s come up enough that I’m open to it.

Maybe filteredBy would read better?

@yairchu

This comment has been minimized.

Show comment
Hide comment
@yairchu

yairchu Jun 5, 2018

Contributor

filteredBy LGTM, shall I create a PR?

Contributor

yairchu commented Jun 5, 2018

filteredBy LGTM, shall I create a PR?

yairchu added a commit to lamdu/lamdu that referenced this issue Jun 5, 2018

@glguy

This comment has been minimized.

Show comment
Hide comment
@glguy

glguy Jun 5, 2018

Collaborator

You'll need to:

  • Document that this is generally going to be an invalid Traversal (a la filtered)
  • Fix the type to have a monomorphic argument instead of a Fold

This could perhaps be improved with the type: filteredBy :: (a -> Maybe b) -> IndexedTraversal' b a a to most carefully capture the behavior. Then the fact that only a single value will be used will be evident from the use of preview if a Fold is being used in the first argument.

Collaborator

glguy commented Jun 5, 2018

You'll need to:

  • Document that this is generally going to be an invalid Traversal (a la filtered)
  • Fix the type to have a monomorphic argument instead of a Fold

This could perhaps be improved with the type: filteredBy :: (a -> Maybe b) -> IndexedTraversal' b a a to most carefully capture the behavior. Then the fact that only a single value will be used will be evident from the use of preview if a Fold is being used in the first argument.

@ekmett

This comment has been minimized.

Show comment
Hide comment
@ekmett

ekmett Jun 5, 2018

Owner

The problem with filteredBy taking (a -> Maybe b) is that then the common usecase still requires a call to preview inside it, exploding it back to the same number of "words" has before with has.

Owner

ekmett commented Jun 5, 2018

The problem with filteredBy taking (a -> Maybe b) is that then the common usecase still requires a call to preview inside it, exploding it back to the same number of "words" has before with has.

@ekmett

This comment has been minimized.

Show comment
Hide comment
@ekmett

ekmett Jun 5, 2018

Owner

filteredBy (preview x) is longer than filtered (has x)

Owner

ekmett commented Jun 5, 2018

filteredBy (preview x) is longer than filtered (has x)

@glguy

This comment has been minimized.

Show comment
Hide comment
@glguy

glguy Jun 5, 2018

Collaborator

That's true that it's longer, but the filteredBy version is supposedly more useful because it puts the extracted value into the index. Access to that value in the index is the only reason I think it's worth defining this in the first place.

Collaborator

glguy commented Jun 5, 2018

That's true that it's longer, but the filteredBy version is supposedly more useful because it puts the extracted value into the index. Access to that value in the index is the only reason I think it's worth defining this in the first place.

@ekmett

This comment has been minimized.

Show comment
Hide comment
@ekmett

ekmett Jun 5, 2018

Owner

@yairchu: Subject to the two bullet points raised by @glguy, sure.

Owner

ekmett commented Jun 5, 2018

@yairchu: Subject to the two bullet points raised by @glguy, sure.

@ekmett

This comment has been minimized.

Show comment
Hide comment
@ekmett

ekmett Jun 5, 2018

Owner

I've been asked for some flavor of this about every 2-3 months for a couple of years now. Exposing the filtered part as an index finally gave me enough of an excuse to say yes. ;)

Owner

ekmett commented Jun 5, 2018

I've been asked for some flavor of this about every 2-3 months for a couple of years now. Exposing the filtered part as an index finally gave me enough of an excuse to say yes. ;)

@yairchu

This comment has been minimized.

Show comment
Hide comment
@yairchu

yairchu Jun 7, 2018

Contributor

I'm stuck on the PR, due to having trouble to make the types work for this simple example which I came up with for the doc-test:

>>> [(Just 2, 3), (Nothing, 4)] <&> filteredBy (_1 . _Just) . _2 %@~ (*)

This neither works with the type mentioned above nor with the more general

filteredBy :: (Indexable i p, Applicative f) => Getting (First i) a i -> p a (f a) -> a -> f a

I'm afraid I haven't yet mastered the different lens types and need some advice here..

(Note that I have been successfully using this combinator in my own code and haven't noticed this issue)

Contributor

yairchu commented Jun 7, 2018

I'm stuck on the PR, due to having trouble to make the types work for this simple example which I came up with for the doc-test:

>>> [(Just 2, 3), (Nothing, 4)] <&> filteredBy (_1 . _Just) . _2 %@~ (*)

This neither works with the type mentioned above nor with the more general

filteredBy :: (Indexable i p, Applicative f) => Getting (First i) a i -> p a (f a) -> a -> f a

I'm afraid I haven't yet mastered the different lens types and need some advice here..

(Note that I have been successfully using this combinator in my own code and haven't noticed this issue)

@glguy

This comment has been minimized.

Show comment
Hide comment
@glguy

glguy Jun 7, 2018

Collaborator

@yairchu _2 is a plain Lens, it's not an IndexPreservingLens, so you need to write filteredBy (_1 . _Just) <. _2 if you want to use the index.

filteredBy :: Getting (First i) a i -> IndexedTraversal' i a a

Collaborator

glguy commented Jun 7, 2018

@yairchu _2 is a plain Lens, it's not an IndexPreservingLens, so you need to write filteredBy (_1 . _Just) <. _2 if you want to use the index.

filteredBy :: Getting (First i) a i -> IndexedTraversal' i a a

@yairchu

This comment has been minimized.

Show comment
Hide comment
@yairchu

yairchu Jun 7, 2018

Contributor

@glguy oh oops :) thanks!

Contributor

yairchu commented Jun 7, 2018

@glguy oh oops :) thanks!

@Peaker

This comment has been minimized.

Show comment
Hide comment
@Peaker

Peaker Jun 12, 2018

Contributor

This is also very useful for indexing one part of the structure while modifying another, without any filtering, e.g if we want to divide a monster's health by its age:

monster .> filteredBy monsterAge <. monsterHealth %@~ (/)

I would also say that traversing via prisms/etc is also a similar kind of "filter" to this one.

Perhaps it would make sense to:

  1. Rename filteredBy to toIndex
  2. Instead of putting the first match into the index, behave like (^.) and put the mconcat of the matches
Contributor

Peaker commented Jun 12, 2018

This is also very useful for indexing one part of the structure while modifying another, without any filtering, e.g if we want to divide a monster's health by its age:

monster .> filteredBy monsterAge <. monsterHealth %@~ (/)

I would also say that traversing via prisms/etc is also a similar kind of "filter" to this one.

Perhaps it would make sense to:

  1. Rename filteredBy to toIndex
  2. Instead of putting the first match into the index, behave like (^.) and put the mconcat of the matches
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment