book-store: Update json for new "input" policy

[rpottsoh]

#996 Updated the schema for canonical data. So I am updating the book-store exercise's json to reflect the change.

[petertseng]

I strongly believe that targetgrouping is not an input and therefore it should not be placed in the input object.

I am assuming that inputs present in this input object are to be provided to the code under test. However, if we were to do this, then the code under test can simply read the targetgrouping to determine what the answer should be, instead of solving the problem of determining the optimal grouping.

I see the following in https://github.com/exercism/problem-specifications/blob/master/exercises/book-store/description.md

Your mission is to write a piece of code to calculate the price of any conceivable shopping basket (containing only books of the same series), giving as big a discount as possible.

Well unfortunately for me, this neither confirms nor denies what I said. It doesn't explicitly say whether the code is responsible for calculating the expected grouping or whether the expected grouping will be provided. So, please decide this.

1. Confirm that the grouping will be provided to the code under test. In that case, I request:

• The basket can simply be removed, it's redundant.
• Shall there be cases that test what happens when non-optimal groupings are provided as input? For example, "targetgrouping": [[1], [2]]?

2. Remove targetgrouping from this file completely.
3. Move targetgrouping to expected, not input. TBD whether tests should request the target grouping instead of just the price (the description does not ask to determine the grouping!)

My preferences:

Remove completely > move to expected > further discussion to discover other options > leave it in input

[coriolinus]

Strongly agree that targetgrouping should not be in input. As I understand it, it's a hint to test writers, but neither input nor expected.

If valid in the new schema, I'd leave it where it is, as an attribute on the individual cases. If the schema prevents this, I'd favor moving it to a comment on each case. This could expose it to students, but that's not actually a problem; it shows very clearly the structure their code is expected to compute from the inputs.

[rpottsoh]

I agree that targetgrouping is not an input, but it isn't expected either, so I was hoping for others to weigh in. 👍 I think moving it to a comment is the best suggestion I have read so far. Second best would be to remove completely.

[petertseng]

If valid in the new schema

Currently uncertain. I could not think of a use for additional properties as of #998 (comment) but I see that this could give us one. So, we could allow additional properties on the top level. Or, comment will be fine.

OK, in that case for me:

Move to comment > Remove completely > leave it where it is (attribute on individual case) > move to expected > further discussion to discover other options > leave it in input

[petertseng]

#1037 (comment) is sufficiently important to me. Feel free to dismiss this when it is decided, I might not be around to!

[coriolinus]

I think all three of us like the idea of moving it into a comment, so let's just do that.

…

On Sat, Dec 16, 2017 at 3:41 AM, Peter Tseng ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In exercises/book-store/canonical-data.json <#1037 (comment)> : > @@ -13,99 +13,127 @@ { "property": "total", "description": "Only a single book", - "basket": [1], - "targetgrouping": [[1]], + "input": { + "basket": [1], + "targetgrouping": [[1]] If valid in the new schema Currently uncertain. I could not think of a use for additional properties as of #998 (comment) <#998 (comment)> but I see that this could give us one. So, we could allow additional properties on the top level. Or, comment will be fine. OK, in that case for me: Move to comment > Remove completely > leave it where it is > move to expected > further discussion to discover other options > leave it in input — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1037 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHdeTsJjzrg1fbt0TBrzHkA1ZyDiF7-0ks5tAy3egaJpZM4RD43-> .

[rpottsoh]

I have had a chance to sleep on it and am now more inclined to just remove targetgrouping. However, I will move them to comments first to see how we like that before taking the more drastic measure. My new reluctance stems from thinking that the targetgrouping may not always represent the only optimal grouping. There could be another grouping that yields the lowest expected cost. Case in point, the last case has:

"targetgrouping": [[1,2,3,4],[1,2,3,5],[1,2,3,4],[1,2,3,5]]

My example solution yields [[1,2,3,4],[1,2,3,5],[1,2,3,5],[1,2,3,4]], perhaps a minor discrepancy, but does result in the expected amount.

In the comments I will consider these as suggested groupings instead of target groupings.

[rpottsoh]

I have inserted comments, but I need some assistance on adding them correctly. The schema doesn't allow for comments to be added in the individual cases.
https://jsonschemalint.com/#/version/draft-04/markup/json?gist=187088f628e3fb188f6bf2b0bc9a31e5

[coriolinus]

I think that may be a bug in the schema; it's entirely valid to want to add comments to individual test cases. Not being very familiar with JSON schemas myself, I'm not sure how exactly to fix this.

One possibility that caught my eye is that AFAICT, the schema expects comments to be lists of strings, not single strings. In that case, surrounding all of the comment strings with brackets would fix the schema validation. Even if that fixes it, I still think that's a schema bug: a comment should be valid as a string or as a list of strings.

@petertseng I think you wrote the schema; I have two questions for you:

1. Have I interpreted the problem here correctly?
2. Is there some reason not to update the schema to allow comments which are single strings instead of lists of strings?

[rpottsoh]

I tried a list of strings, didn't seem to help. Reading the schema. Comments do not appear to be included in the allowed list of fields of a test

[rpottsoh]

Furthermore, the current and proposed versions of the schema have this as a possible issue.

we have decided what to do with targetgrouping

[petertseng]

I have inserted comments, but I need some assistance on adding them correctly. The schema doesn't allow for comments to be added in the individual cases.
Comments do not appear to be included in the allowed list of fields of a test

Yes it does by https://github.com/petertseng/exercism-problem-specifications/blob/fd0db38d87816e21f554b250592789a3d3600c9d/schema-with-input.json#L89 in the proposed schema and https://github.com/exercism/problem-specifications/blob/master/canonical-schema.json#L91 in the current schema, and by the explicit example of a comment in an individual case in https://github.com/exercism/problem-specifications/blob/master/exercises/largest-series-product/canonical-data.json#L89-L102

I tried a list of strings, didn't seem to help.

https://jsonschemalint.com/#/version/draft-04/markup/json?gist=98985e3ceec42789416324414cc9982d validates. I didn't want to change every single "comments" so I reduced it to one. I leave it to others to update the others.

@petertseng I think you wrote the schema

This is important enough for me to need to shut this down now. I did not write the schema and unless I deleted a comment in #602 and subsequently forgot about it, I had nothing to do with the schema my only involvement with the schema was to 👍 it.

the schema expects comments to be lists of strings, not single strings. In that case, surrounding all of the comment strings with brackets would fix the schema validation

I agree with this assessment and have provided evidence of its truth above.

Is there some reason not to update the schema to allow comments which are single strings instead of lists of strings?

I am not aware of any reason to refrain from allowing strings.

[rpottsoh]

@petertseng thanks for your feedback and pointing out examples that show the proper use of comments. I'll work on updating my changes.

[coriolinus]

This is important enough for me to need to shut this down now. I did not write the schema and unless I deleted a comment in #602 and subsequently forgot about it, I had nothing to do with the schema my only involvement with the schema was to 👍 it.

@petertseng I assumed wrongly; sorry. I saw that you authored #998 and made assumptions. Do you consider the scope of #998 to include the proposed change of allowing single string comments instead of only list comments, or would that more properly fit into a separate PR?

[petertseng]

Do you consider the scope of #998 to include the proposed change of allowing single string comments instead of only list comments, or would that more properly fit into a separate PR?

I surely can add it. But I cannot guarantee when #998 will be merged. At the very least I need to add the use-old-schema files and then make Travis CI decide which schema is correct to use based on the presence or absence of that file. No guarantees on when that will happen.

If it is useful for this change to be included in the schema at any time before #998 is merged, it would certainly be possible for an interested party to change https://github.com/exercism/problem-specifications/blob/master/canonical-schema.json without waiting for #998.

Questions have come up and they need resolution.

[rpottsoh]

I am not sure if the wording of the comments that I have added is sufficient? If anyone has any objections or would otherwise like to see the wording changed in some way, please let me know.

I would like to squash & merge this PR in the next day or so. I don't see a need to squash.