-
Notifications
You must be signed in to change notification settings - Fork 24
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
Support for explicit downcasting indexed optics to regular ones #44
Conversation
optics-core/src/Optics/Indexed.hs
Outdated
@@ -1,13 +1,13 @@ | |||
module Optics.Indexed | |||
( | |||
-- * Functors with index | |||
FunctorWithIndex (..) | |||
IxFunctpr (..) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functpr
Is renaming to avoid clashes with lens
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, I just figured that since we have IxProfunctor and indexed optics start with Ix, classes can be the same for consistency.
Constraints 'A_Setter p = Mapping p | ||
Constraints 'An_IxSetter p = p ~ IxFunArrow | ||
Constraints 'A_Traversal p = IxTraversing p | ||
Constraints 'An_IxTraversal p = IxTraversing p |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel uneasy about this.
Do we still have
traverseOf traversed = traverse
traverseOf (traversed % traversed) = traverse . traverse
(I think inspection-testing
)
But if this works, then #41 can be tweaked even further into nicer form indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, nothing changes with regard to operations on regular optics, the extra constraint is just so that indexed variants can be used as regular ones (as Ix* instances for Forget, Star and FunArrow simply ignore indices).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you mean code generation? I didn't check.
@arybczak could you pull the "Ranem indexed classes" (fixing the typo) into own PR so we could merge it right away (If that's something we want to do, I have no strong opinion about) |
Right, I'll do that later today. Yes, we want explicit (not implicit though as in lens) downcasting. |
@arybczak I'll push inspection tests very soon. |
@arybczak I just merged the inspection tests I mentioned, if they still pass with your patch, I think this is a good way forward. |
83e0be1
to
3ee6926
Compare
Looks like they pass. I force pushed the UnindexableOptic class and removed the commit that renames the classes. |
ada8986
to
3ee6926
Compare
3ee6926
to
7fbe35c
Compare
Proof of concept for #25 for now (in particular I think we should have a class with associated function
unIx
instead ofunIxTraversal
etc.I think it can look better when representing indices as in #41, then regular versions have to accept optics with any list of indices instead of two free type parameters.
@phadej thoughts?