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

Expose derived lenses as associated constants #352

Merged
merged 1 commit into from
Nov 25, 2019

Conversation

Ralith
Copy link
Collaborator

@Ralith Ralith commented Nov 25, 2019

Generates lens type definitions inside a private module with an unlikely-to-collide name, then uses those to populate associated constants on the type for extremely ergonomic access. The main drawback here is that the private __Foo_druid_lenses module name pops up in error messages.

@Ralith Ralith added the discussion needs feedback and ideas label Nov 25, 2019
@Ralith
Copy link
Collaborator Author

Ralith commented Nov 25, 2019

Updated to define the lenses in a public module with some documentation. This makes things a little prettier; it's not perfect but I think it's pretty good.

Example error from using a lens of the wrong type:

error[E0277]: the trait bound `impl druid::Widget<f64>: druid::Widget<bool>` is not satisfied
  --> druid/examples/slider.rs:53:37
   |
53 |     col.add_child(Padding::new(5.0, slider), 1.0);
   |                                     ^^^^^^ the trait `druid::Widget<bool>` is not implemented for `impl druid::Widget<f64>`
   |
   = note: required because of the requirements on the impl of `druid::Widget<DemoState>` for `druid::lens::LensWrap<bool, demo_state_lenses::double, impl druid::Widget<f64>>`
   = note: required by `druid::widget::padding::Padding::<T>::new`

@cmyr
Copy link
Member

cmyr commented Nov 25, 2019

My only nit might be to add generated or derived to the name of the module; so something like demo_state_generated_lenses or demo_state_derived_lenses?

@Ralith
Copy link
Collaborator Author

Ralith commented Nov 25, 2019

We'll be able to get rid of the public module when rust-lang/rust#63065 comes along.

@cmyr
Copy link
Member

cmyr commented Nov 25, 2019

want to make that name tweak? then I'm happy to merge, this is a solid improvement.

@Ralith
Copy link
Collaborator Author

Ralith commented Nov 25, 2019

Done, plus some brief documentation.

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great, thanks, appreciate the attention to detail.

@cmyr cmyr merged commit 5b54d7f into linebender:master Nov 25, 2019
@cmyr cmyr mentioned this pull request Dec 31, 2019
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion needs feedback and ideas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants