Swaps all `_` only parameters for `input` #474

Baelyk · 2018-03-17T20:17:04Z

Resolves #442

Changed the parameter to input, and uses it in unimplemented!() 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 was already changed)

IanWhitney · 2018-03-17T20:33:59Z

Thanks! You're close. I think if update the { } to {:?} the tests should pass.

Baelyk · 2018-03-17T21:05:49Z

Okay! The tests now pass!

petertseng · 2018-03-17T21:39:17Z

Resolves #438

Please use #442 instead

petertseng · 2018-03-17T22:00:49Z

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.

I'm happy to reduce the scope of #442 to say only change the exact variable name _. As we can see in #438, the problem was it's not obvious to the student that it's necessary to change _ since it can never be referenced.

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 #[allow(unused_variables)] that we see.

Baelyk · 2018-03-17T22:27:54Z

Oops I meant 442...

Is it okay if I open that issue?

petertseng · 2018-03-17T22:30:30Z

Oops I meant 442...

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.

Is it okay if I open that issue?

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

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

Baelyk · 2018-03-17T23:08:41Z

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

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 {:?}.

petertseng · 2018-03-17T23:20:58Z

exercises/alphametics/src/lib.rs

-pub fn solve(_: &str) -> Option<HashMap<char, u8>> {
-    unimplemented!()
+pub fn solve(input: &str) -> Option<HashMap<char, u8>> {
+    unimplemented!("Solve the alphametric {:?}", input)


alphametric contradicts the spelling of the exercise, so the r should be removed

(I'm OK with this being input).

petertseng · 2018-03-17T23:21:41Z

exercises/book-store/src/lib.rs

@@ -1,3 +1,3 @@
-pub fn lowest_price(_: &[usize]) -> usize {
-    unimplemented!()
+pub fn lowest_price(input: &[usize]) -> usize {


input doesn't tell me what the meaning of the input is. I'd like to see books.

petertseng · 2018-03-17T23:22:06Z

exercises/crypto-square/src/lib.rs

@@ -1,3 +1,3 @@
-pub fn encrypt(_: &str) -> String {
-    unimplemented!()
+pub fn encrypt(input: &str) -> String {


I'm OK with this being input.

Another possibility I would accept is plaintext.

petertseng · 2018-03-17T23:22:21Z

exercises/decimal/src/lib.rs

@@ -4,7 +4,7 @@ pub struct Decimal {
 }

 impl Decimal {
-    pub fn try_from(_: &str) -> Option<Decimal> {
-        unimplemented!()
+    pub fn try_from(input: &str) -> Option<Decimal> {


I think I'm OK with input here too.

petertseng · 2018-03-17T23:22:43Z

exercises/poker/src/lib.rs

@@ -2,6 +2,6 @@
 ///
 /// Note the type signature: this function should return _the same_ reference to
 /// 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>> {


I'd like this to tell me what each element of the slice is, so I'll say hands or input_hands

petertseng · 2018-03-17T23:25:03Z

exercises/decimal/src/lib.rs

-    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)


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 `{}`.

petertseng

Cool, unless I missed something, I'm 👍 on all of this.

I request the one who merges this squash the commits.

IanWhitney · 2018-03-18T15:40:57Z

Squashed & merged. Thanks for the help @Baelyk!

petertseng suggested changes Mar 17, 2018

View reviewed changes

Swaps all _ only parameters for input

3bdeb6e

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

Baelyk force-pushed the 442 branch from 181b7b5 to 3bdeb6e Compare March 17, 2018 23:07

Baelyk mentioned this pull request Mar 17, 2018

Variable names that start with an underscore should be removed #476

Closed

3 tasks

Actually used {:?} this time

4ae7e55

petertseng suggested changes Mar 17, 2018

View reviewed changes

Variable names have meaning

1b6a52a

Stopped using just `input` where something else made more sense. Changed decimal from `{:?}` back to `{}`.

petertseng approved these changes Mar 17, 2018

View reviewed changes

IanWhitney merged commit b3b23f6 into exercism:master Mar 18, 2018

Swaps all _ only parameters for input #474

Swaps all _ only parameters for input #474

Uh oh!

Conversation

Baelyk commented Mar 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IanWhitney commented Mar 17, 2018

Uh oh!

Baelyk commented Mar 17, 2018

Uh oh!

petertseng commented Mar 17, 2018

Uh oh!

petertseng commented Mar 17, 2018

Uh oh!

Baelyk commented Mar 17, 2018

Uh oh!

petertseng commented Mar 17, 2018

Uh oh!

petertseng left a comment

Choose a reason for hiding this comment

Uh oh!

Baelyk commented Mar 17, 2018

Uh oh!

petertseng left a comment

Choose a reason for hiding this comment

Uh oh!

petertseng Mar 17, 2018

Choose a reason for hiding this comment

Uh oh!

petertseng Mar 17, 2018

Choose a reason for hiding this comment

Uh oh!

petertseng Mar 17, 2018

Choose a reason for hiding this comment

Uh oh!

petertseng Mar 17, 2018

Choose a reason for hiding this comment

Uh oh!

petertseng Mar 17, 2018

Choose a reason for hiding this comment

Uh oh!

petertseng Mar 17, 2018

Choose a reason for hiding this comment

Uh oh!

petertseng left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IanWhitney commented Mar 18, 2018

Uh oh!

Uh oh!

Swaps all `_` only parameters for `input` #474

Swaps all `_` only parameters for `input` #474

Baelyk commented Mar 17, 2018 •

edited

Loading

petertseng left a comment •

edited

Loading