Swaps all _ only parameters for input#474
Conversation
|
Thanks! You're close. I think if update the |
|
Okay! The tests now pass! |
I'm happy to reduce the scope of #442 to say only change the exact variable name In that case, we'd also want to open a separate issue to change the underscore-prefixed variable names, because it is although it is easy to see how to reference those variables, it is undesirable. By convention underscore-prefixed names indicate unused variables, and the only reason the underscores are present is to silence unused variable warnings. In a similar vein, we'd like to remove any |
|
Oops I meant 442... Is it okay if I open that issue? |
Don't forget to amend your commit message as well (not just your pull request description, which you have now edited). If not, I request the person who merges this please remember to change the issue number from 438 to 442.
I must ask to make sure it's ambiguous. You mean you want to open an issue that asks for underscore-prefixed variable names to be removed? If so, cool. |
petertseng
left a comment
petertseng
left a comment
There was a problem hiding this comment.
I am looking at the existing stubs using this pattern:
$ git grep -B1 'unimplemented!("'
exercises/gigasecond/src/lib.rs-pub fn after(start: DateTime<Utc>) -> DateTime<Utc> {
exercises/gigasecond/src/lib.rs: unimplemented!("What time is a gigasecond later than {}", start);
--
exercises/nth-prime/src/lib.rs-pub fn nth(n: u32) -> Option<u32> {
exercises/nth-prime/src/lib.rs: unimplemented!("What is the {}th prime number?", n)
--
exercises/prime-factors/src/lib.rs-pub fn factors(n: u32) -> Vec<u32> {
exercises/prime-factors/src/lib.rs: unimplemented!("This should calculate the prime factors of {}", n)
--
exercises/reverse-string/src/lib.rs-pub fn reverse(input: &str) -> String {
exercises/reverse-string/src/lib.rs: unimplemented!("Write a function to reverse {}", input);
These:
- tell what to do either phrased as a question or a command.
- use names indicating the meaning of the input, rather than simply telling us it's the input.
I'd like these stubs to do the same, rather than simply say "the input variable", which provides no guidance.
I understand that plain unimplemented!() provides no guidance as well, so simply saying "the input variable" makes things no worse than they already are, but I think if we solve this issue halfway it will be hard to tell where we solved it halfway and where we solved it all the way, so we might as well solve it all the way wherever we solve it.
Resolves exercism#442 Changed the parameter to `input`, and uses it in `unimplemented!()` to silence the unused variable warning. Use `{:?}` instead `{}` Meaningful filler for the unimplemented!() macros
|
Okay I updated the unimplemented text and squashed it down to one commit (I couldn't find another way to amend the original commit). |
petertseng
left a comment
petertseng
left a comment
There was a problem hiding this comment.
I thought about my request about input and I decided I'm actually OK with input for the scalar types. For slice inputs I want to know what each element of the slice is, so I asked for names accordingly. I also suggested one place where I think {} is better than {:?}.
| pub fn solve(_: &str) -> Option<HashMap<char, u8>> { | ||
| unimplemented!() | ||
| pub fn solve(input: &str) -> Option<HashMap<char, u8>> { | ||
| unimplemented!("Solve the alphametric {:?}", input) |
There was a problem hiding this comment.
alphametric contradicts the spelling of the exercise, so the r should be removed
(I'm OK with this being input).
| @@ -1,3 +1,3 @@ | |||
| pub fn lowest_price(_: &[usize]) -> usize { | |||
| unimplemented!() | |||
| pub fn lowest_price(input: &[usize]) -> usize { | |||
There was a problem hiding this comment.
input doesn't tell me what the meaning of the input is. I'd like to see books.
| @@ -1,3 +1,3 @@ | |||
| pub fn encrypt(_: &str) -> String { | |||
| unimplemented!() | |||
| pub fn encrypt(input: &str) -> String { | |||
There was a problem hiding this comment.
I'm OK with this being input.
Another possibility I would accept is plaintext.
| impl Decimal { | ||
| pub fn try_from(_: &str) -> Option<Decimal> { | ||
| unimplemented!() | ||
| pub fn try_from(input: &str) -> Option<Decimal> { |
There was a problem hiding this comment.
I think I'm OK with input here too.
| /// the winning hand(s) as were passed in, not reconstructed strings which happen to be equal. | ||
| pub fn winning_hands<'a>(_: &[&'a str]) -> Option<Vec<&'a str>> { | ||
| unimplemented!() | ||
| pub fn winning_hands<'a>(input: &[&'a str]) -> Option<Vec<&'a str>> { |
There was a problem hiding this comment.
I'd like this to tell me what each element of the slice is, so I'll say hands or input_hands
| pub fn try_from(_: &str) -> Option<Decimal> { | ||
| unimplemented!() | ||
| pub fn try_from(input: &str) -> Option<Decimal> { | ||
| unimplemented!("Create a new decimal with a value of {:?}", input) |
There was a problem hiding this comment.
I think I prefer you use {} here, since it will say Create a new decimal with a value of 1.1 rather than Create a new decimal with a value of "1.1" and I'd rather see it without the quotes.
Stopped using just `input` where something else made more sense.
Changed decimal from `{:?}` back to `{}`.
|
Squashed & merged. Thanks for the help @Baelyk! |
3 participants
Resolves #442
Changed the parameter to
input, and uses it inunimplemented!()to silence the unused variable warning.Some of the exercises reported in the issue used
_as a prefix and not as a lone parameter (e.g. in isbn-verifier,_isbn). I wasn't if those were also a problem, so I didn't change them.Changed exercises:
reverse-string(reverse-string was already changed)