Skip to content

Commit

Permalink
fix(enums3): Update Message::ChangeColor to take a tuple. (#457)
Browse files Browse the repository at this point in the history
  • Loading branch information
@yukihane
yukihane authored Jul 8, 2020
1 parent 816b1f5 commit 4b6540c
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion exercises/enums/enums3.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ mod tests {
position: Point{ x: 0, y: 0 },
color: (0, 0, 0)
};
state.process(Message::ChangeColor(255, 0, 255));
state.process(Message::ChangeColor((255, 0, 255)));
state.process(Message::Echo(String::from("hello world")));
state.process(Message::Move(Point{ x: 10, y: 15 }));
state.process(Message::Quit);
Expand Down

13 comments on commit 4b6540c

@lucasrizoli
Copy link

@lucasrizoli lucasrizoli commented on 4b6540c May 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the double-parens a typo? Seems like it's off and should be as previously, state.process(Message::ChangeColor(255, 0, 255));

Sorry if I'm missing something; as you might've guessed, I'm still learning Rust. 😄

@qsmodo
Copy link

@qsmodo qsmodo commented on 4b6540c Jul 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any input on this?

@dmavelar
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong to me, I was stuck on this exercise FOREVER because I didn't want to change that line. I just started learning Rust and have no idea how to make it work in this current state.

@lucasrizoli
Copy link

@lucasrizoli lucasrizoli commented on 4b6540c Jul 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I was able to solve the exercise without editing this line. That is, Message::ChangeColor((255, 0, 255))) is not a mistake—I just considered it counterintuitive. (Nice to know I'm not the only one.)

@dmavelar
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagined it would be possible, I guess I'm still learning and can't figure it out.

@lucasrizoli
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My solution (spoilers!) 😝 https://gist.github.com/lucasrizoli/88c5b01b035da2940ad314cb90364cbf

@kellan
Copy link

@kellan kellan commented on 4b6540c Nov 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to vote for this also being counter intuitive, and very different than what someone reading through the Rust book (me!) would expect.

@shadows-withal
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, tuple types are clearly explained in the book here: https://doc.rust-lang.org/book/ch03-02-data-types.html#the-tuple-type, but I'll add a comment to make it more clear that this is intentional.

@HerschelW
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tuples are clearly explained in the rust-lang book... none of which have double parenthesis in and of themselves. Yes there are the ((t, u, p, l, e), (t, u, p, l, e)) and (key: (t, u, p, l, e,)) occurrences which are intuitive and to be expected, but certainly not doubled for no reason whatsoever.

@shadows-withal
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's true, I don't have a particular preference on how these should be written, rustfmt doesn't enforce one style or the other, so I just deferred it to what the commit author used. I don't have strong feelings on this either way, I feel like leaving it like this could be an interesting way to communicate "Hey look, you can also define tuples like this!", but I wouldn't mind reverting it either.

@HerschelW
Copy link

@HerschelW HerschelW commented on 4b6540c Dec 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, either adding more clarity to the comment or removing the extra parenthesis is adequate. The main confusion is that as it stands, you're required to either change the tuple struct or change the process function in order to pass when doing either of those isn't actually conducive to the exercise itself.

I can put in a pull request if you'd like.

@shadows-withal
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

@HerschelW
Copy link

@HerschelW HerschelW commented on 4b6540c Dec 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

Added a pull request. The circumstances surrounding tuples and additional parentheses are in regard to passing an argument to a function. Otherwise, I could not find any reference stating that extra parentheses indicate a tuple.

If I create a tuple such as let tup = (('a',)) I receive the following warning:

unnecessary parentheses around assigned value '#[warn(unused_parens)] on by default'

Furthermore if I made an attempt to indicate a tuple with a single element by doing double parentheses
let tup = (('a'))
the rustfmt will automatically remove the extra parentheses and assume I meant to indicate a char type. Similar to Python, the comma after the lone element is what will indicate the value being a tuple. ('a',)

PR: #1310

Please sign in to comment.