-
Notifications
You must be signed in to change notification settings - Fork 531
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 more stubs (pythagorean-triplet, perfect-numbers) #370
Conversation
Oops, there is |
Thanks.
Correct, it does not.
I'd like to ask the commit message to be amended because "Update" is too vague. A function stub was added, so the commit message should say that. |
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 for these stubs!
exercises/perfect-numbers/src/lib.rs
Outdated
@@ -0,0 +1,7 @@ | |||
pub enum Classification { | |||
|
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 request that the three possible variants of Classification
be included in the stub, since there is no point in keeping them from the student; the test expects them and the description lists them already.
The other reason is that the stub will not compile without it.
The precedent for this request is in https://github.com/exercism/rust/blob/master/exercises/forth/src/lib.rs#L7-L12 and https://github.com/exercism/rust/blob/master/exercises/robot-simulator/src/lib.rs#L4-L10.
exercises/perfect-numbers/src/lib.rs
Outdated
} | ||
|
||
pub fn classify(num: u64) -> Result<Classification, &'static 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.
there should be unimplemented!()
here, otherwise the stub will not compile because the compiler thinks ()
is being returned
The reason for this request is so that cargo test -- --no-run
can be run on this stub, for the purpose of automatically ensuring that the signature provided in the stub is correct (I am unable to manually ensure this, because of time).
@@ -0,0 +1,4 @@ | |||
pub fn find() -> Option<u32> { | |||
|
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.
there should be unimplemented!()
here, otherwise the stub will not compile because the compiler thinks ()
is being returned
The reason for this request is so that cargo test -- --no-run
can be run on this stub, for the purpose of automatically ensuring that the signature provided in the stub is correct (I am unable to manually ensure this, because of time).
Very good, thank you! Will you do us the favour of combining the commits such that the final state is two commits:
We wouldn't want this repo to have intermediate commits where there are stubs that are not runnable (Ask if you need advice on how to achieve it. One might think of various ways like an interactive rebase or reset, add, commit, add, commit) |
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.
indeed, this has been done. We are ready to merge.
Thank you for contributing these stubs! |
No description provided.