Implement lens composition #344
Conversation
Ralith
force-pushed
the
lens-ext
branch
3 times, most recently
from
af4b3ce to
417311a
Compare
|
Rebased and made independent. |
|
Rebased back onto #341 for the sake of tests, renamed |
druid/src/lens.rs
Outdated
| } | ||
| } | ||
|
|
||
| /// `Lens` for an offset value |
There was a problem hiding this comment.
I'm not convinced of the general usefulness of Mul and Add here. I can see their use for the temperature conversion 7guis example, but can't easily think of other uses. So I think they make sense as an example of what lenses can do, but prefer not to have them in the main docs.
There was a problem hiding this comment.
These are motivated by my own real-world use, where I want a slider to control a value whose range is not 0-1. They're particularly convenient because they bake in the inverse operation, which is an error/correctness risk otherwise. I'll see if I can maybe expand Iso into something more convenient for this case while retaining generality.
There was a problem hiding this comment.
Ahhh right. I wonder whether longer term we want to do it this way (emphasizing the simplicity of individual components and composing them), or make the slider more configurable. But yes, that is an additional use case, and argues in favor of including them.
| }; | ||
| } | ||
|
|
||
| /// `Lens` composed of two lenses joined together |
There was a problem hiding this comment.
This is definitely one of the most important combinators, thanks! I think I'm good with "Then", but was also thinking about names more reminiscent of functional composition.
There was a problem hiding this comment.
I'm open to suggestions! This has some precedent, with Option::and_then and moreso FutureExt::then.
| } | ||
|
|
||
| impl<T, U, B: ?Sized> Then<T, U, B> { | ||
| pub fn new<A: ?Sized, C: ?Sized>(left: T, right: U) -> Self |
There was a problem hiding this comment.
The docstring should probably point to the then method. Also, I'm not sure "left" and "right" are clear names, though I can't think of more authoritative ones off the top of my head. (I feel like I should read a primer on lenses or compositional mathematic operations)
I know in Haskell it's just function composition, but that doesn't seem available here.
druid/src/lens.rs
Outdated
| } | ||
|
|
||
| /// `Lens` for indexing containers | ||
| pub struct Index<I> { |
There was a problem hiding this comment.
I'm impressed we can use the Rust Index trait - I was kinda thinking we'd need separate ones for Vec, etc. I am wondering, though, what we need to punch through the Arc in the common Arc<Vec> combination.
There was a problem hiding this comment.
I think a deref combinator is possible. Alternatively, for better ergonomics maybe Deref can be baked directly into this? I'll experiment some tonight.
There was a problem hiding this comment.
Neither of those approaches admit a reasonable way to inject the Arc::make_mut, so probably a different tactic is needed for that.
| @@ -26,7 +26,8 @@ mod command; | |||
| mod data; | |||
| mod env; | |||
| mod event; | |||
| mod lens; | |||
| pub mod lens; | |||
There was a problem hiding this comment.
I think we don't want the modules in the namespace, just the items such as Lens and so on. I can see some possible issues with the intermediate structs, in which case we should probably be using pub(crate) if impl Lens will work as a type, or failing that, #[doc(hidden)].
There was a problem hiding this comment.
impl Lens is unfortunately impossible in an extension trait for the time being. I'm not sure how I feel about going out of our way to hide these types; what's the upside?
There was a problem hiding this comment.
It's not so much that we want to hide the types, as that we want to avoid modules being exported directly into the namespace. This is recent reorg work by @cmyr and maybe he has stronger feelings about it.
There was a problem hiding this comment.
Personally, I like pub mod foo { struct Bar; } when I would otherwise have to do struct FooBar;, which I think is the case here; the lens names are pretty vague out of context.
There was a problem hiding this comment.
I'm 100% okay with some public modules in the case where there is a clear argument for them, e.g. they have tons of exports and it is clarifying to keep them separate, and this sounds like a example of that.
I mostly just don't want public modules to be the default choice.
Ralith
force-pushed
the
lens-ext
branch
3 times, most recently
from
e162f67 to
a7ebc14
Compare
Ralith
changed the title
|
Stripped this down to the obviously correct portion; will continue to explore more advanced stuff in one or more new PRs. |
Ralith
force-pushed
the
lens-ext
branch
4 times, most recently
from
451f10d to
f0b3278
Compare
|
Renamed the |
No description provided.