-
Notifications
You must be signed in to change notification settings - Fork 27
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
Some suggestions for changes (which I can implement) #21
Comments
Hi thanks for these points.
I think this is a good suggestion. I personally prefer boxed trait objects, as you say it will remove the lifetimes.
I don't think currently we are at a stage where we can stablalise API yet. In the long run, as you say it would be good to move really common structs and traits in to the top level namespace, or perhaps include a
I'm not personally keen on having
Perhaps e.g. the
I am undecided. Text editors support autocomplete so it's not the typing that's the problem. Long names lead to long lines which are more difficult to read. |
I've been having a look into your first item, changing references to trait objects into owned trait objects ( master...mindriot101:boxed-traits In addition, for ergonomics I included Some things I noticed:
|
I removed my previous comment, to write again. I implemented the suggestions related to styles in #29. Your second comment:
|
Most of these suggestions, I will already implement locally as I work on my project (which uses plotlib to plot stuff). I will submit a PR after a while. Until then, I open this issue so that you may discuss or protest against some of my suggestions. I can then change or revert that point. So please state your opinion!
I will keep it updated as I think of new things.
I think that taking
Box<MyTrait>
rather than&'a MyTrait
in general makes a better programming interface, eliminating lifetimes. Examples:Page::add_plot
andContinuousView::add
both take references to traits. In fact I was forced to change the latter function toadd(mut self, repr: Box<ContinuousRepresentation>)
because of lifetime issues in my project (more specifically, I created the data in a loop. The old solution would require me to store the data in a Vec and then loop again over that Vec to plot the data). It remains, however, to change toBox
in other functions. Done in Start with legends, and less references #40[PR Make 3 common style structs, remove style traits #29] Naming: It's rather confusing to have
struct line::Line
,struct line::Style
and alsotrait style::Line
. I think we should find some way to rename them. Especially try to not make theLine
s unique only through the module paths. We could for example name themstruct Line
,struct LineStyle
andtrait LineStyleTrait
.[PR Make 3 common style structs, remove style traits #29] Remove the
Style
traits. I don't see the need for a trait for every kind of Style. The only one that has two implementors isstyle::Line
. However these two implementors are identical.Naming: There are some opportunities for shorter names. Some words are quite commonly shortened, such as
s/Representation/Repr
.svg: Rounding errors in Y labels #18
The text was updated successfully, but these errors were encountered: