Add exercise book-store #390
Conversation
This was a fun challenge to implement in Rust, but it also serves as a useful demo of the code generation implemented in exercism#389: past implementing the example, I had to edit five lines of code in tests/book-store.rs and three lines to add the stub in src/lib.rs. Everything else was generated automatically. Comments are invited pertaining both to this exercise and to the output of the exercise generator.
|
Looks like the generator made a bunch of edits to |
exercises/book-store/example.rs
Outdated
| book_groups.push(Group::new_containing(*book)); | ||
| } | ||
| book_groups.sort(); | ||
| println!("DecomposeGroups::new() --> {:?}", book_groups); |
There was a problem hiding this comment.
No, but it does get captured by the test runner, so I didn't bother to take it out. That said, I don't object to removing it.
| /// All cases for the `total` property are implemented | ||
| /// in terms of this function. | ||
| /// | ||
| /// Note that you'll need to both name the expected transform which |
There was a problem hiding this comment.
Should it be the case that the student still sees this? They may think the "you" refers to them.
There was a problem hiding this comment.
I'll take out the comments aimed at the exercise implementor.
| /// Note that you'll need to both name the expected transform which | ||
| /// the student needs to write, and name the types of the inputs and outputs. | ||
| /// While rustc _may_ be able to handle things properly given a working example, | ||
| /// students will face confusing errors if the `I` and `O` types are not concrete. |
There was a problem hiding this comment.
Should it be the case that the student still sees this? They may be confused because I and O are nowhere in sight here.
| /// While rustc _may_ be able to handle things properly given a working example, | ||
| /// students will face confusing errors if the `I` and `O` types are not concrete. | ||
| /// | ||
| /// Expected input format: ('basket', 'targetgrouping') |
There was a problem hiding this comment.
Should it be the case that the student still sees this? They may be confused because basket and targetgrouping are nowhere in sight here
There was a problem hiding this comment.
This is a note to the exercise implementor, but in this case I think we should leave it for clarity for those students who examine the tests.
On test generation, the generator collects the cases. If the case doesn't already have an input property, it generates one as a tuple containing all the unknown properties in the case. However, this discards information: as properties, these items have names; as tuple fields, they do not. The comment is inserted to help the implementer decipher the input tuple.
There was a problem hiding this comment.
I was about to suggest
Ah, then this should be
process_total_case(basket: Vec<usize>, targetgrouping: Vec<Vec<usize>>, expected: f64)
but now I hesitate a little bit about that suggestion because then it might not be clear which of these are inputs, whereas it is clar if they're named input:
It's also unfortuntae because targetgrouping is not an input. Maybe it shouldn't be in the canonical-data.json at all. But we can continue this discussion pretending it's an input, based on what we would like to see for exercises that actually do have multiple inputs.
There was a problem hiding this comment.
The principle I used when writing the generator is that in cases where we require manual intervention from the test implementor, the effort required should be minimal. The actual generated code looks like this:
fn process_{property}_case<I, O>(input: I, expected: O) {
// typical implementation:
// assert_eq!(
// student_{property}_func(input),
// expected
// )
unimplemented!()
}While it would be possible to break that out into a more complex function signature, as you say, it becomes less obvious for the test implementer which parameters form the actual inputs. I prefer to leave them as a simple tuple, for now.
| /// students will face confusing errors if the `I` and `O` types are not concrete. | ||
| /// | ||
| /// Expected input format: ('basket', 'targetgrouping') | ||
| fn process_total_case(input: (Vec<usize>, Vec<Vec<usize>>), expected: f64) { |
There was a problem hiding this comment.
I note that input.1 is never used, but I'm not particularly concerned. Might make one wonder "why not just not pass it into this function, then?"
There was a problem hiding this comment.
As commented above, the input tuple contains all the case properties which we don't know. In this case, I believe that the targetgrouping field was meant to be an output hint.
I considered having the student return their groups and comparing them to the target groupings, but I decided against it. Checking the computed lowest price only gives the same assurance as checking it with the groups, but with a considerably simpler interface.
This was a fun challenge to implement in Rust, but it also serves
as a useful demo of the code generation implemented in #389:
past implementing the example, I had to edit five lines of code
in tests/book-store.rs and three lines to add the stub in src/lib.rs.
Everything else was generated automatically.
Comments are invited pertaining both to this exercise and to the output
of the exercise generator.