-
Notifications
You must be signed in to change notification settings - Fork 303
Recipe Series: Sort Vector #437 #444
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
Conversation
BTW: #443 The test failures are from a rust-lang bug |
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.
Thanks @Januson , some minor suggestions follow.
|
||
Sorts a Vector of Person structs with a single property `name` by its natural | ||
order (By name). In order to make Person sortable you need four traits [`Eq`], | ||
[`PartialEq`], [`Ord`] and [`PartialOrd`]. Eq can be simply derived. |
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 that the proposed examples are quite verbose for relatively trivial functionality that is shown.
I'd at least suggest to combine the last two examples src/algorithms/sorting/sort_struct_custom.md and src/algorithms/sorting/sort_struct_natural.md
Also these last two examples are more focused in actually making the values sortable as opposed to actual sorting. This is not bad in itself but I fell that title does not does the example justice as the actual sorting seams to be the least important aspect of the example
I'd also mention that Eq
, PartialEq
, Ord
and PartialOrd
can be just #[derive]
'd. Moreover I'd only do the impl
's by hand if these do something interesting justifying the human work otherwise #[derive]
is always a better / more idiomatic choice (which we should prefer), So to summarize I'd either use auto derives or make the struct more interesting to justify the manual impls.
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.
Hello, thanks for your suggestions. I merged the struct sorting examples and derived the structs.
|
||
fn main() { | ||
let expected = vec![ | ||
Person::new("Al".to_string(), 60), |
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'd propose to move the whole body of expected
to the right side of assert_eq
as in the previous examples
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 inlined the expected variables.
* Add simple sorting vector of integers * Add sorting of vector of floats * Add sorting of vector of structs
e990665
to
e374220
Compare
I'm pretty happy with the way this turned out. |
fixes #437