Port selected functions from Control.Lens.Plated by arybczak · Pull Request #379 · well-typed/optics

arybczak · 2020-12-25T13:31:10Z

It needs a bit of polish wrt. documentation. Do we want to have it in 0.4?

michaelpj

I would definitely like to have these!

michaelpj · 2021-06-19T12:45:49Z

optics-core/src/Optics/Plated.hs

+
+-- | Rewrite recursively over part of a larger structure.
+rewriteOn :: (Is k A_Setter, Plated a) => Optic k is s t a a -> (a -> Maybe a) -> s -> t
+rewriteOn b = over b . rewrite


I have wondered in the past why these functions exist (in lens). Composing rewrite with over b seems like something we could expect our users to do. After all, a user of a lens library is probably pretty used to stringing together combinations of transformations.

Maybe we can do without the *On variants.

I copied the API of lens verbatim since I never used Plated, but you're right, it makes sense to provide MVP and extend if necessary.

michaelpj · 2021-06-19T13:03:55Z

optics-core/src/Optics/Plated.hs

+-------------------------------------------------------------------------------
+
+-- | Perform a fold-like computation on each value, technically a paramorphism.
+paraOf :: Is k A_Fold => Optic' k is a a -> (a -> [r] -> r) -> a -> r


I don't know to what degree we want to mimic the exact structure of lens, but arguably it would make sense to put the *Of variants in the modules for the optics they use, e.g. paraOf in Fold, traverseOf in Setter etc, leaving this module to contain the versions that actually use Plated.

(Which I just noticed is exactly how it is for partsOf already.)

Indeed. I moved paraOf to Optics.Fold and removed para and parts for now.

Oh, I didn't read this suggestion well enough it seems. Yeah, having Optics.Plated only have the functions with plate applied also sounds reasonable.

michaelpj · 2021-06-19T13:11:44Z

optics-core/src/Optics/Plated.hs

+universeOf :: Is k A_Fold => Optic' k is a a -> a -> [a]
+universeOf l = go
+  where
+    go a = a : foldMapOf l go a


I know lens doesn't do this either, but is there a reason not to define universe* fucntions in terms of cosmos* and toListOf?

I have no idea, sadly I never used these functions so have no intuition.

Boarders · 2021-10-31T04:26:09Z

Any updates on this PR? I'd be happy to help get it over the line.

mikeplus64 · 2021-11-01T03:47:07Z

I wonder if Plated would be better off in its own package or in recursion-schemes where it IMO fits in very well. It's a really useful typeclass and it would be nice if lens and optics had compatibility with it as well.

class Plated a where
  plate :: Applicative f => (a -> f b) -> s -> f t

I don't know if that's impossible with optics' Traversal.

arybczak · 2021-12-07T17:20:34Z

I've updated the PR and stripped the API down. Could you tell me if that's ok? I've never used Plated so can't really tell if the API is now much harder to use.

I'm even wondering about removing Plate and all functions without the Of suffix that just apply the Of variant to plate. But maybe that's too much?

arybczak · 2021-12-07T18:01:37Z

I'm even wondering about removing Plate and all functions without the Of suffix

The last commit performs that experiment. It looks the most reasonable to me 🤔 Thoughts?

arybczak · 2021-12-07T18:09:58Z

@mikeplus64 this is a good idea. optics can reuse Plated in the form you've written by simply wrapping it in traversalVL.

The only problem is the default instance would be missing, which might or might not be a big deal.

phadej · 2021-12-07T18:59:37Z

In my opinion Plated type-class shouldn't be in lens and shouldn't be copied to optics either. It's just a traversal for immediate children. (It would be silly to have Traversable in lens, and we pulled TraversableWithIndex out of it too).

EDIT: I'm 👍 with adding non Plated specific combinators. I'm 👎 for adding the class.

arybczak · 2021-12-07T19:56:46Z

@phadej I'm with you on this one. I don't see the point of having Plated and *Of variants that are independent of it.

I think this PR can be merged as-is.

michaelpj

This looks great to me, very neat!

phadej · 2021-12-08T16:52:20Z

optics-core/src/Optics/Setter.hs

+
+-- | Transform every element by recursively applying a given 'Setter' in a
+-- bottom-up manner.
+transformOf :: Is k A_Setter => Optic k is a b a b -> (b -> b) -> a -> b


Is it deliberate that the *OnOf variants are left out? They are useful when you e.g. want to transform all Pats inside Exp:

transformOnOf patExps expPlated modifyPat :: Exp -> Exp where patExps :: Setter' Pat Exp expPlated :: Setter' Pat Pat modifyPlated :: Pat -> Pat

Of course I can do over patExps (transformOf expPlated modifyPlated)...

Of course I can do over patExps (transformOf expPlated modifyPlated)...

Yeah, that's what was also mentioned in #379 (comment), I think these are a bit excessive.

We can always add them later if there is enough demand.

phadej · 2021-12-08T16:53:06Z

optics-core/src/Optics/Fold.hs

+universeOf :: Is k A_Fold => Optic' k is a a -> a -> [a]
+universeOf l = go
+  where
+    go a = a : foldMapOf l go a


this is slow for trees. ++ can easily make this quadratic. I don't know why DList = Endo [a] is not used here (and in lens) instead.

Can you give an example?

I don't know why DList = Endo [a] is not used here (and in lens) instead.

Me neither, I assume since lens has it like that it's fine 🤔 It can be optimized later if necessary.

See ekmett/lens#994

I updated PR to reflect changes made to lens.

arybczak force-pushed the plated branch from ec4c785 to 418efe8 Compare December 25, 2020 13:32

michaelpj reviewed Jun 19, 2021

View reviewed changes

fishtreesugar mentioned this pull request Oct 31, 2021

Plated #435

Closed

arybczak added 2 commits December 7, 2021 18:09

Add Optics.Plated

48f268f

Thin out the API

0de33b0

arybczak force-pushed the plated branch from 418efe8 to 0de33b0 Compare December 7, 2021 17:18

Remove Optics.Plated and move functions to their appropriate modules

1c7de0c

Restore Optics.Plated

6ab3da5

arybczak changed the title ~~[WIP] Add Optics.Plated~~ Port selected function from Control.Lens.Plated Dec 7, 2021

arybczak changed the title ~~Port selected function from Control.Lens.Plated~~ Port selected functions from Control.Lens.Plated Dec 7, 2021

Remove Optics.Plated for good

9e94a9f

arybczak requested a review from phadej December 7, 2021 19:57

michaelpj approved these changes Dec 8, 2021

View reviewed changes

phadej reviewed Dec 8, 2021

View reviewed changes

Better universeOf and consistent variable names

8b9b199

arybczak merged commit 18ad71d into master Dec 21, 2021

arybczak deleted the plated branch December 21, 2021 01:49

Conversation

arybczak commented Dec 25, 2020

Uh oh!

michaelpj left a comment

Choose a reason for hiding this comment

Uh oh!

michaelpj Jun 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arybczak Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Boarders commented Oct 31, 2021

Uh oh!

mikeplus64 commented Nov 1, 2021

Uh oh!

arybczak commented Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arybczak commented Dec 7, 2021

Uh oh!

arybczak commented Dec 7, 2021

Uh oh!

phadej commented Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arybczak commented Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelpj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phadej Dec 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

michaelpj Jun 19, 2021 •

edited

Loading

arybczak Dec 7, 2021 •

edited

Loading

arybczak commented Dec 7, 2021 •

edited

Loading

phadej commented Dec 7, 2021 •

edited

Loading

arybczak commented Dec 7, 2021 •

edited

Loading

phadej Dec 8, 2021 •

edited

Loading