brackets: simplify required interface #620
Conversation
There was a problem hiding this comment.
I could suggest taking a look at #178 to see why it was written the way it is, but even without looking at it, I know I prefer the interface proposed in this PR since it is less restrictive. Behind the scenes, the student may still submit an implementation that creates a struct, if desired. This is exemplified by the fact that the example does just that. I prefer to give the choice to the student, so I want this change to be enacted.
Please see my note about the version change, to be dealt with before merging.
| @@ -1,88 +1,88 @@ | |||
| extern crate bracket_push; | |||
|
|
|||
| use bracket_push::*; | |||
| use bracket_push::brackets_are_balanced; | |||
There was a problem hiding this comment.
note: If the crate name were brackets, I would have wanted the name to be brackets::are_balanced to avoid repetition. However, looks like we are using bracket_push. So I think brackets_are_balanced has to be the name.
It can be worth a discussion whether to change bracket_push (why is it even called bracket-push??? exercism/problem-specifications#693 ) but this PR does not need to depend on it.
exercises/bracket-push/Cargo.toml
Outdated
| @@ -1,3 +1,3 @@ | |||
| [package] | |||
| name = "bracket-push" | |||
| version = "1.2.0" | |||
| version = "1.3.0" | |||
There was a problem hiding this comment.
this should not change until the test cases match https://github.com/exercism/problem-specifications/blob/master/exercises/bracket-push/canonical-data.json 1.3.0. It should go in a separate PR.
There was a problem hiding this comment.
Ah ok, so that's what it's used for. Prescribed test cases also affects two of my other PRs which are trying to add more.
|
I've reverted the change to the version. |
There was a problem hiding this comment.
I found one more note about the variable name, perhaps we can get that changed before merging? awaiting your comment on it.
After that, I don't see anything else I suggest to change, so once I get that I'm going to Approve. I don't know how many downloads this exercise gets daily but I hope to be snappy about this one. So once someone other than me Approves, or if nobody else does then 48 hours since PR submission?
exercises/bracket-push/src/lib.rs
Outdated
| pub fn are_balanced(&self) -> bool { | ||
| unimplemented!("Check if your Brackets struct contains balanced brackets"); | ||
| } | ||
| pub fn brackets_are_balanced(_string: &str) -> bool { |
There was a problem hiding this comment.
I understand that the leading underscore is to say it is OK that it is unused.
I would like to suggest instead the model established in #438 - call it string and reference it in the unimplemented message. The benefit of the approach is that it avoids a potential student misunderstanding, thinking that the variable should remain named _string.
Otherwise, this will fall under #476 which asked us to remove all instances of variables with leading underscores
There was a problem hiding this comment.
Yeah, that sounds reasonable. It felt wrong to put it there in the first place to get past the (denied) warning.
The Brackets struct has a single purpose: To allow `is_balanced` to be called. It forces an object-style without any advantages over a simple function.
|
Thanks! |
I'm just going to quote Peter Tillemans' post from the slack channel. He put it very nicely.
The hint in the Readme points students towards using lifetimes, but most just accept the
impl From<&'a str> for Bracketsas is. They don't work with lifetimes themselves, they only conform to the interface.