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

lens usability improvements, examples ? #62

Closed
simonmichael opened this issue Jun 8, 2016 · 12 comments
Closed

lens usability improvements, examples ? #62

simonmichael opened this issue Jun 8, 2016 · 12 comments

Comments

@simonmichael
Copy link
Contributor

simonmichael commented Jun 8, 2016

(micro)lens seems desirable for working with nested records in a brick app. I just started to exploring this, and am finding it harder than expected. Notes so far:

  • The first thing I want to use is (^.). Apparently this requires that the types it accesses are monoids. My records have brick Lists in them. You can't derive a Monoid instance for List because the constructors are hidden. I defined one manually for now:
instance Monoid (List a)
  where
    mempty      = list "" V.empty 1
    mappend a b = a & listElementsL .~ (a^.listElementsL <> b^.listElementsL)
  • I found myself wanting listSelectedElementL (and wrongly guessing listSelectedL was it).
@simonmichael
Copy link
Contributor Author

simonmichael commented Jun 8, 2016

  • using listSelectedL seems difficult sometimes, again because of this Monoid issue. I thought this would work:
>>> :t accountsScreen ^. asState . asItems
accountsScreen ^. asState . asItems :: List AccountsScreenItem
>>> :t accountsScreen ^. asState . asItems . listSelectedL

<interactive>:1:19:
    No instance for (Monoid Int) arising from a use of ‘asState’
    In the first argument of ‘(.)’, namely ‘asState’
    In the second argument of ‘(^.)’, namely
      ‘asState . asItems . listSelectedL’
    In the expression:
      accountsScreen ^. asState . asItems . listSelectedL

@simonmichael
Copy link
Contributor Author

simonmichael commented Jun 8, 2016

But this works:

>>> :t (accountsScreen ^. asState . asItems) ^. listSelectedL
(accountsScreen ^. asState . asItems) ^. listSelectedL :: Maybe Int

@simonmichael
Copy link
Contributor Author

It might be a misunderstanding of lens principles on my part. I can see how an accessor following one that returns multiple values could need to be a monoid.. but asItems gets a singular value - a List - so I'm confused.

@simonmichael
Copy link
Contributor Author

And here are the current type definitions: https://gist.github.com/simonmichael/f75813855f56c54b08e19f89155517da

@jtdaugherty
Copy link
Owner

^. doesn't require a Monoid instance inherently; it depends on what is on the right hand side. The Haddock docs for ^. give an example. However! I learned something new, which is that the Monoid instance requirement is coming from the fact that you're generating a partial lens, asState, on the Screen type. microlens is being smart about lens generation: if you were to use asState on a Screen value that wasn't the right constructor, it would need to fail -- and it would use mempty of the appropriate Monoid to do so.

To do this it generates Lenses when they are total and Traversals when they are partial, and ^. requires a Monoid instance when used with a Traversal.The following code sample reproduced this for me (notice the use of ^? with the traversal instead of ^.):

data Testing =
    First { _thing :: Int
          , _other :: Int
          }
    | Second { _other :: Int
             }
makeLenses ''Testing

and the types of the lenses generated:

*Main Brick.Widgets.List Lens.Micro> :i other
other :: Lens' Testing Int
*Main Brick.Widgets.List Lens.Micro> :i thing
thing :: Traversal' Testing Int

and so:

*Main Brick.Widgets.List Lens.Micro> (Main.First 1 2) ^? thing
Just 1
*Main Brick.Widgets.List Lens.Micro> (Main.Second 3) ^? thing
Nothing
*Main Brick.Widgets.List Lens.Micro> (Main.Second 3) ^. other
3

Regarding the missing Monoid instance: List doesn't provide a Monoid instance because it's not really possible to say what an instance would look like in general if two lists have different item heights or names, and it isn't possible to say what mempty should look like. It also isn't possible to make such a Monoid obey the Monoid law mempty <> x = x since if the left hand side takes precedence the equation is false. You can define one for your app as long as it provides the semantics you expect, but I can't provide one in the library for this reason.

simonmichael added a commit to simonmichael/hledger that referenced this issue Jun 9, 2016
Experimenting with lenses to reduce record accessing/updating noise.
So far, it's not at all a clear win.
cf jtdaugherty/brick#62
@simonmichael
Copy link
Contributor Author

simonmichael commented Jun 9, 2016

That is very clarifying, thank you very much!

I would like to get rid of those partial record fields, but I haven't found a better solution yet.

I wonder how you are seeing those nice types in GHCI (Lens' & Traversal'). I get the more verbose:

*Hledger.UI.UITypes *Hledger.UI.Main Lens.Micro> :t asList
asList
  :: Applicative f =>
     (List AccountsScreenItem -> f (List AccountsScreenItem))
     -> Screen -> f Screen

with GHC 7.10.3 and the latest types.

@jtdaugherty
Copy link
Owner

Now that we've explored what was going on here, is there anything remaining that you need me to address? Or can this be closed?

@simonmichael
Copy link
Contributor Author

Feel free to close it. I don't know if adding listSelectedElementL is appropriate.

@jtdaugherty
Copy link
Owner

Right now listSelectedElement gives you back Maybe (Int, e). It sounds from your original request that you might want the lens-compatible version to give back something similar. Making this be a Setter would be tricky but it would be easy to make a Getter with this type:

listSelectedElementL :: SimpleGetter (List n e) (Maybe (Int, e))

so that you could then do

let maybeSel = someList^.listSelectedElementL
case maybeSel of
  Nothing -> ...
  Just _ -> ...

Is this what you have in mind?

@jtdaugherty
Copy link
Owner

@simonmichael - this ticket has been open for a while. Do you still have anything specific you'd like to see here? Or can I close it?

@simonmichael
Copy link
Contributor Author

Close away, thank you @jtdaugherty .

@jtdaugherty
Copy link
Owner

Thanks!

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