exercise: book-store, canonical data#470
exercise: book-store, canonical data#470rpottsoh merged 21 commits intoexercism:masterfrom rpottsoh:rpottsoh-patch-2
Conversation
Create harry-potter
Create description.md
Create description.md
initial version
|
This looks like a fun exercise. We're going to need a Here's an example: https://github.com/exercism/x-common/blob/master/exercises/nucleotide-count/metadata.yml |
Already provided here: #465 |
rpottsoh
changed the title
There was a problem hiding this comment.
Just bringing in some previous discussions to this PR. I also echo @Insti 's thought that the descriptions should tell about why the test is important #451 (comment)
The descriptions of "TestBasketWithNDifferentBooks" I actually think are OK, but I would think the descriptions are written as prose, such as "test basket with N different books". For "basket 11223345" I would say something like "two groups of four are better than groups of five and three".
| "description": "Calculate lowest price for shopping basket only containing books from a single series", | ||
| "cases": [ | ||
| { | ||
| "description": "TestSingleBookPrice", |
There was a problem hiding this comment.
previous discussion #465 (comment)
In retrospect this probably isn't needed. I have a property that just returns the price of a single book. The test is just confirming that it equals 8.
I would agree. Not every implementation necessarliy needs to have this property in order to be correct. We would prefer not to drive the implementation.
| "expected": 8.0 | ||
| }, | ||
| { | ||
| "description": "TestBasketContainsMyBooks", |
There was a problem hiding this comment.
previous discussion: #465 (comment)
I was confused about this one since the key is "mybasket" instead of "basket". Is this a significant difference?
In addition, I don't think it is particularly necessary for this problem that a basket can report what it contains - the test code is already telling the basket what it contains. It seems the tests should be interested in finding out the price of a basket (as all the other tests are).
There was a problem hiding this comment.
@petertseng I agree. I am going to clean this up a bit. This initial version of the json file is essentialy just a dump of the tests. I was not sure at the time what exactly should appear in the json file or what its purpose is. As I have stated before this is my first time trying to add an exercise, so learning the ropes.
| }, | ||
| { | ||
| "description": "basket 112233445512", | ||
| "basket": "112233445512", |
There was a problem hiding this comment.
previous discussion: #465 (comment) - would be good if this were an array
String type comes with the added benefit of TStringHelper which makes parsing the basket a little easier.
Sorry, I'm not familiar with what a TStringHelper is nor what language it's associated with. We'd like the canonical JSON files to be independent of any one language.
| "Test a basket containing two copies of book 1, ", | ||
| "two copies of book 2, two copies of book 3, ", | ||
| "and one copy each of books 4 and 5." | ||
| ], |
There was a problem hiding this comment.
This isn't describing why this test is useful. It's just a verbose English language description of the input.
| { | ||
| "description": "TestBasketWith2DifferentBooks", | ||
| "description": "Test basket with two 2 different books.", | ||
| "basket": "12", |
There was a problem hiding this comment.
Basket should be an array rather than a string.
Another attempt at re-writing descriptions. Also converted baskets to arrays.
| }, | ||
| { | ||
| "description": "basket 112233445512", | ||
| "basket": "112233445512", |
There was a problem hiding this comment.
@petertseng now I am not sure that I need the "basket" token. I have been cleaning up the test runner and the basket is now injected when a new basket is created. There is no longer a basket property that needs to be assigned after instantiating the basket. Please refer to the book store PR if you need to see what I mean.
There was a problem hiding this comment.
If we suppose that it is not needed: What would you propose each case in this JSON file should look like?
| { | ||
| "description": [ | ||
| "Should arrive at best discount and correct total for a basket containing 8 books.", | ||
| "The basket will consist of two each of the first 3 books and one each of books 4 and 5." |
There was a problem hiding this comment.
I can just as easily find out this information by examining the basket. I would like something that tells me why this case is important.
This is a great case to discuss because the description.md calls it out specifically. It says that you could try to group it as a group of five plus a gorup of three or alternatively two groups of four, and that the latter is better.
So a description that I would like to see for this is simply "two groups of four books are cheaper than a group of five plus a group of three". Don't need to include the "Should arrive at best discount and correct total..." either, that's already implied by the problem and it's the same for all the test cases, we don't want to be repeating ourselves.
There was a problem hiding this comment.
@petertseng when/how is the canonical data presented to the student? I think it would help me if I knew the context of when each of the three (description.md, canonical-data.json, and metadata.yml) files is used.
There was a problem hiding this comment.
when/how is the canonical data presented to the student?
Never.
It is to be used by anyone who wishes to implement this exercise for a track for which it is not implemented yet.
The old way of doing it was that everyone would have to look at every other track's test files and figure out what the best set of tests is. With this new way, everyone only has to look in one place: here.
Trying to follow @PeterSeng suggestions.
|
@PeterSeng thanks this is helpful, thanks.
|
Descriptions was too specific
|
How is the decision made to go ahead and merge the PR? |
petertseng
left a comment
petertseng
left a comment
There was a problem hiding this comment.
How is the decision made to go ahead and merge the PR?
It seems the standard for this repo (from what I've seen anyway) is it just needs one person other than the submitter to review it and OK it. Sometimes if I am the submitter and I think a PR can be contentious I'll wait a few days after the first person OKs it to give anyone else a chance. Maybe we can/should document this somewhere.
In general: I'm not sure I understand the sentence of the form "A basket containing X is the least expensive grouping"... because a grouping (how the books should be put into groups and their price calculated) only makes sense in the context of a given basket. So if I read, "A basket containing X is the least expensive grouping", my mind asks the question "the least expensive grouping for what??"
I would expect it would be something of the form "For a basket containing X, the least expensive grouping is Y".
| "expected": 8.0 | ||
| }, | ||
| { | ||
| "description": "A basket containing only two of the same book is charged full price.", |
There was a problem hiding this comment.
sorry, mind making this align? there's one space missing, compared to "basket" below.
| { | ||
| "description": [ | ||
| "A basket containing three pairs of three different books plus ", | ||
| "one copy each of the other two books, not paired, is the least expensive ", |
There was a problem hiding this comment.
I don't think the "other two books, not paired" in this description is accurate. The other two books do get grouped - the 4 gets grouped with one [1, 2, 3] set, and the 5 gets grouped with the other [1, 2, 3] group. The wording seems to imply that the groups in this basket would be [1, 2, 3], [1, 2, 3], [4], [5], when in fact it is [1, 2, 3, 4], [1, 2, 3, 5]. I would like to see it made clear that [1, 2, 3, 4], [1, 2, 3, 5] is the best grouping, rather than [1, 2, 3, 4, 5], [1, 2, 3] (or any other).
| { | ||
| "description": [ | ||
| "A basket containing four pairs of four different books plus ", | ||
| "one copy of a book, not already paired, is the least expensive grouping." |
There was a problem hiding this comment.
this "not already paired" also seems to imply that the grouping for this basket would be [1, 2, 3, 4], [1, 2, 3, 4], [5] when in fact it is [1, 2, 3, 4, 5], [1, 2, 3, 4].
There was a problem hiding this comment.
@petertseng thanks for the feedback. Spacing issue should be resolved. I was totally missing that the descriptions should contain the solution that yields the expected result, duh. I have re-written the descriptions. I left them wordy. If you think being too wordy is harder to follow I could, for example, rewrite to be like given basket [1,1,2,2,3,3,4,4,5] group as [1,2,3,4,5] and [1,2,3,4].... Let me know what you think.
|
Great that you're working on a set of canonical test data @rpottsoh! |
I was not understanding the the descriptions should relate the solution that achieves the expected result. I am not sure having the descriptions this wordy is desirable.
petertseng
left a comment
petertseng
left a comment
There was a problem hiding this comment.
I am not sure having the descriptions this wordy is desirable.
Indeed. Well, all we want is something that tells implementors these two things:
- What is being tested
- Why it is important to test
For this reason, I should not have suggested "For a basket containing X, the least expensive group is Y", because the possible values of Y already imply what X is.
I do like to keep test descriptions short. Do see other canonical-data.json for examples.
| @@ -0,0 +1 @@ | |||
|
|
|||
There was a problem hiding this comment.
don't forget to remove this file
There was a problem hiding this comment.
@petertseng help me out here. This file doesn't appear here. I clearly see the empty file in the PR, but do not know how to remove it other than to close the PR and open a new one.
There was a problem hiding this comment.
You will not find it in https://github.com/rpottsoh/x-common/tree/rpottsoh-patch-2/exercises/book-store .
You will find it in https://github.com/rpottsoh/x-common/tree/rpottsoh-patch-2/exercises/harry-potter .
| "expected": 8.0 | ||
| }, | ||
| { | ||
| "description": [ |
There was a problem hiding this comment.
spacing doesn't look right to me either. ![http://imgur.com/swFco3s] ![http://imgur.com/UJWeSnj]
There was a problem hiding this comment.
Hopefully spacing is OK now. I need to see if I can get notepad++ to insert a set number of spaces when tab is pressed, instead of an actual tab.
| }, | ||
| { | ||
| "description": [ | ||
| "A basket containing only two of the same book is most inexpensively ", |
There was a problem hiding this comment.
If you want to shorten this: There is no other grouping possible for this basket, so simply "Basket with two of the same book has only one possible grouping" could work, right?
| }, | ||
| { | ||
| "description": [ | ||
| "For a basket containing eight books consisting of a pair each of the first three books ", |
There was a problem hiding this comment.
This could be shortened to "Two groups of four is cheaper than a group of five plus a group of three". This tells us why we have this test - to catch a common mistake students might make (always make the largest group possible)
Can probably make similar changes for all others.
Removed all tabs and replaced with appropriate number of spaces.
petertseng
left a comment
petertseng
left a comment
There was a problem hiding this comment.
In its current state, I would support this JSON file.
Reminder to squash commits.
The door is open, of course, to shorten those test descriptions if desired. I'm happy to take another look if that happens.
|
@petertseng Did you know you can do the squashing from GitHub? Just use the "Squash and merge" option when merging the PR. |
|
@petertseng I am working to cut down on the verbage of each description. I am using the .json file for the Change exercise. I am introducing a new field, "targetgrouping" that has array data for the least expensive grouping to achieve the "expected" outcome. |
Cut down the wordiness of each description. Introduced "targetgrouping" field. This field contains the grouping that yields the most discount to the customer.
I've got that distinct feeling of deja vu. #338 (comment) |
| { | ||
| "description": "A basket containing only a single book.", | ||
| "basket": [1], | ||
| "targetgrouping": [1], |
There was a problem hiding this comment.
compare the type of targetgrouping in the below case: an array of arrays of integers: [[2], [2]], versus this one here, an array of integers: [1].
Should it be [[1]] for consistency?
Same question for all other cases where the type is array of integer rather than array of arrays of integers.
There was a problem hiding this comment.
Yes it should be [[1]] for consistency. Change has been made.
| }, | ||
| { | ||
| "description": [ | ||
| "A basket containing eight books consisting of a ", |
There was a problem hiding this comment.
Any way to make it clear either in the description or elsewhere that we test this case to make sure that the student finds [[1, 2, 3, 4], [1, 2, 3, 5]] rather than [[1, 2, 3, 4, 5], [1, 2, 3]]?
It's interesting to me because I think this is the only case in this JSON file where the greedy solution won't work, so it's particularly important to me that the description point that out.
There was a problem hiding this comment.
I am making two changes to address that having more than any one book in a group is of no advantage to the customer and the other change to draw the readers attention to this special case, that is counter-intuitive to how one might group the books, as you have pointed out.
rpottsoh
dismissed
petertseng’s stale review
Still making changes. Re-evaluate. I'll squash merge after your next approval.
"targetgrouping" should be an array of arrays. Also modified two descriptions to make this exercise clearer to the reader.
petertseng
left a comment
petertseng
left a comment
There was a problem hiding this comment.
I say this looks good as-is (some implementors of this exercise might choose to shorten the descriptions more when porting to their track, but we will know that when they implement).
There is one thing I would like to ask about: [[]] versus [], take a look.
| { | ||
| "description": "No charge to carry around an empty basket.", | ||
| "basket": [], | ||
| "targetgrouping": [[]], |
There was a problem hiding this comment.
I actually think [] is OK here (and better than [[]]), since [] still is of the type "array of arrays of integers", it just happens to be an empty array of arrays of integers (it could be an empty array of anything,, really). Why do I think [] is better than [[]]? Well, we don't say [[2], [2], []] right? So we never have empty inner arrays. Thus, [] rather than [[]].
There was a problem hiding this comment.
I can do that. It kind of looked weird to me too.
Addresses issue "Remove rubocop magic comments from user facing files #385"
5 participants
initial canonical-data.json for book-store.