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 toAlternativeOf? #1031

Closed
sjshuck opened this issue Jun 3, 2023 · 7 comments
Closed

Add toAlternativeOf? #1031

sjshuck opened this issue Jun 3, 2023 · 7 comments

Comments

@sjshuck
Copy link
Contributor

sjshuck commented Jun 3, 2023

I use this function without issues:

-- | A more general `toListOf` that collects targets into any `Alternative`.
toAlternativeOf :: (Alternative f) => Getting (Alt f a) s a -> s -> f a
toAlternativeOf l = let alt = Alt . pure in getAlt . view (l . to alt)

https://github.com/sjshuck/hs-pcre2/blob/2d01498056569905abc643fdffa697efbbd6c08c/src/hs/Text/Regex/Pcre2/Internal.hs#L130-L132

I use it as a simple way to unify "find-all" (f ~ []) and "find-one" (f ~ Maybe) in a single implementation. It will attempt to fill up the Alternative or run out of targets, whichever comes first. But I don't have much insight into performance implications. Maybe there's some kind of Endo-based implementation that's possible and preferable (à la toListOf?

Any other considerations?

@ekmett
Copy link
Owner

ekmett commented Jul 6, 2023

The main consideration is that

toAlternativeOf :: Alternative f => Getting (Alt f a) s a -> s -> f a

should be weakened to

toAlternativeOf :: Applicative f => Getting (Alt f a) s a -> s -> f a

then it will only require an Applicative when applied to a Getter, but will require Alternative when used with a Fold (because the Getting itself will require it for Alts Applicative).

@sjshuck
Copy link
Contributor Author

sjshuck commented Jul 10, 2023

Thanks for the feedback.

I'll prepare a PR. Meanwhile, I'm conflicted about the name toAlternativeOf, now moreso because Alternative doesn't exist anywhere in the type.

Possibilities:

  • toAltOf (because Alt is in the type)
  • toAOf ("A" as in sequenceA or asum)
  • toPureOf/pureOf (because really what it does is call pure on targets, and if through a Fold, <|>s them together)

Honestly I like toPureOf/pureOf.

@ekmett
Copy link
Owner

ekmett commented Jul 10, 2023

I admit I'm not a big fan of lots of camelCase. I'd probably go altOf.

@ekmett
Copy link
Owner

ekmett commented Jul 10, 2023

derp. this is asumOf

@sjshuck
Copy link
Contributor Author

sjshuck commented Jul 10, 2023

I think it's asumOf (l . to pure) that also support Applicative for single-target optics.

@ekmett
Copy link
Owner

ekmett commented Jul 10, 2023

Yeah. I guess altOf is probably the right thing then. I'm not a big fan of coining new names if I can avoid it, but I'm much less a fan of toXyzOf proliferating.

"Also supporting a weaker constraint for single target optics" is generally a thing, sumOf and productOf already do that.

@sjshuck
Copy link
Contributor Author

sjshuck commented Jul 10, 2023

Right, whereas view, which is named after the weaker action ("just look at a thing"), has this sometimes surprising behavior of mushing targets together if you accidentally request the stronger action by having multiple targets—altOf gives you a heads-up that something mushy is liable to happen. 🙂

@sjshuck sjshuck mentioned this issue Jul 10, 2023
sjshuck added a commit to sjshuck/lens that referenced this issue Jul 10, 2023
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

No branches or pull requests

2 participants