Skip to content

replace Stepper::min/max with 'with_range' #684

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

Closed
cmyr opened this issue Mar 17, 2020 · 2 comments · Fixed by #697
Closed

replace Stepper::min/max with 'with_range' #684

cmyr opened this issue Mar 17, 2020 · 2 comments · Fixed by #697
Labels
D-Easy just needs to be implemented good first issue requires little knowledge of druid's internals help wanted has no one working on it yet

Comments

@cmyr
Copy link
Member

cmyr commented Mar 17, 2020

I think these two methods are slightly too generally named, and I would like to be consistent with the new method on Slider.

@cmyr cmyr added help wanted has no one working on it yet good first issue requires little knowledge of druid's internals D-Easy just needs to be implemented labels Mar 17, 2020
@luleyleo
Copy link
Collaborator

I've just started to get this done, but renaming it to with_range feels kind of weird when the next line is step(..) and not with_step(..).
Currently there are many methods not using the builder style naming with_ and it feels like this change would make the naming less consistent.
I think it would be best to first decide whether to use builder style naming for all of none of the methods. My favorite would be naming it just range(.., ..) as the with_ prefix really only makes sense when there are equivalent none builder methods on the same struct.

@cmyr
Copy link
Member Author

cmyr commented Mar 18, 2020

yep, with_step is also welcome.

My current feeling is that builder-style methods that take an argument should be with_. I have two main concerns: one is just consistency, and having a clear convention for naming, and the other is that overly generic method names are likely to collide with other things (for instance, there's a problem with colliding with derived lens names)— but also specifically that with WidgetExt if we have overly generic method names and those are auto-impl'd for a bunch of types, it becomes quite likely there will be a collision on some type, which will cause hard to diagnose compiler errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D-Easy just needs to be implemented good first issue requires little knowledge of druid's internals help wanted has no one working on it yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants