-
Notifications
You must be signed in to change notification settings - Fork 520
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
Add two-bucket test #375
Add two-bucket test #375
Conversation
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.
First, I want to thank you for submitting this exercise! It's great to see you contributing.
I've made a few line comments discussing particular code improvements I'd like to see; both are pretty simple changes.
You'll also need to placate Travis before we can merge this. In particular, this PR currently fails because you haven't included this exercise in config.json
. That file determines where this exercise will go in the sequence, and what difficulty it will be assigned. I recommend looking for other exercises which seem similar in difficulty to this one, then putting in the appropriate difficulty number. Don't worry that there isn't an objective answer; there's always some subjectivity, and as long as you pick something broadly plausible and internally consistent, nobody will give you any flak over it. You can use any tool you like to generate a new UUID.
exercises/two-bucket/src/lib.rs
Outdated
_goal: u8, | ||
_start_bucket: &str) -> BucketStats | ||
{ | ||
return BucketStats { |
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.
It's better form to have the body of the stub function be simply { unimplemented!() }
, to make it easy for people to see where they should begin work.
exercises/two-bucket/src/lib.rs
Outdated
/// the first fill. | ||
pub moves: u8, | ||
/// Which bucket should end up with the desired number of liters? (Either "one" or "two") | ||
pub goal_bucket: &'a str, |
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.
Let's leverage Rust's capabilities, and use a two-element enum
to represent the final bucket. It's more type-safe, and a better example for learners.
Thanks for the really quick feedback! I should have at least caught the config.json thing from the README.md, but I guess I missed the section on linting somehow. Anyway, everything should be updated now. |
No problem; thanks for making the requested changes quickly. I've marked up a few more things; it all amounts to using Once that change is made, I'll be ready to approve this. I won't merge immediately--I want to give the other maintainers time to take a look and make their own comments, as desired--but if there's no further discussion after that, I'd merge this not later than Friday, 3 Oct. |
exercises/two-bucket/src/lib.rs
Outdated
pub fn solve(capacity_1: u8, | ||
capacity_2: u8, | ||
goal: u8, | ||
start_bucket: &str) -> BucketStats |
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 think this should be an &Bucket
also; this prevents the need for a potential panic in your example implementation. It'll mean updating the tests, but that's fine.
exercises/two-bucket/README.md
Outdated
|
||
Your program should determine: | ||
- the total number of "moves" it should take to reach the desired number of liters, including the first fill - expects a numeric value | ||
- which bucket should end up with the desired number of liters (let's say this is bucket A) - expects a String (either 'one' or 'two') |
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.
This should also be updated to refer to a Bucket
object.
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.
Looks great to me! I'll wait to give other people the chance to chime in, but unless other blockers appear, I'll merge this on Friday.
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.
nice work. just the UUID and README to change, then.
README is (other than the strings bits I mentioned and will deal with) as generated ✔️
Tests are as problem-specifications 1.0.0 ✔️
config.json
Outdated
@@ -647,6 +647,18 @@ | |||
] | |||
}, | |||
{ | |||
"uuid": "e2b2654f-0f8c-1580-35d1-06a7173a61b9b66d44d", |
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.
the last portion is too long (should be 12 hexadecimal digits but has 19 by my count), so this is not a valid https://en.wikipedia.org/wiki/Universally_unique_identifier (I am assuming this is what a UUID is)
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.
Sorry. I generated this with configlet, but I hadn't seen that it might not generate valid UUIDs. This should be fixed now.
exercises/two-bucket/README.md
Outdated
- the size of bucket one, passed as a numeric value | ||
- the size of bucket two, passed as a numeric value | ||
- the desired number of liters to reach, passed as a numeric value | ||
- which bucket to fill first, passed as a String (either 'one' or 'two') |
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.
for the Rust track, this is no longer a string.
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.
Good catch. Updated it to indicate that it takes a Bucket.
exercises/two-bucket/README.md
Outdated
|
||
Your program should determine: | ||
- the total number of "moves" it should take to reach the desired number of liters, including the first fill - expects a numeric value | ||
- which bucket should end up with the desired number of liters (let's say this is bucket A) - expects a Bucket (either Bucket::One or Bucket::Two) |
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.
Because this line differs from problem-specifications and cannot simply be fixed by adding HINTS.md, I dislike this aspect of problem-specifications. I am going to deal with it there. Not a blocker for this PR.
This test is listed in the problem-specifications repo ([1]), but hadn't been added to the rust track yet. This commit adds the description, test cases, and a sample solution based on breadth-first search. [1] https://github.com/exercism/problem-specifications/blob/master/exercises/two-bucket
) Note that two-bucket was added to the Rust track at #375 At the time it declared 1.0.0 compliance, but was actually compliant with 1.2.0 already! Further updates to 1.3.0 and 1.4.0 require no action on the Rust track's part. 1.0.1: Fix exercise name exercism/problem-specifications#715 Unversioned(!): Change descriptions exercism/problem-specifications#716 (Our descriptions are no better or worse, so might as well keep them) 1.1.0: Add cases where goal equals one bucket's capacity exercism/problem-specifications#763 Rust already had these cases, so just update the descriptions 1.2.0: Fix a test case added in 1.1.0 exercism/problem-specifications#911 exercism/problem-specifications#941 Rust already had the correct values, so no action. 1.3.0: move inputs to `input` object exercism/problem-specifications#1084 1.4.0: rename JSON keys to camelCase exercism/problem-specifications#1136
This test is listed in the problem-specifications repo ([1]), but hadn't been
added to the rust track yet. This commit adds the description, test
cases, and a sample solution based on breadth-first search.
[1] https://github.com/exercism/problem-specifications/blob/master/exercises/two-bucket