Fix grade-school

[wolf99]

Example implementation here: exercism/c#683

Fixes #1824

Wasn't sure how to replace the existing grade-school exercise so have just used grade-school2 (should this be grade-school-2?) as had been suggested in slack (https://exercism-team.slack.com/archives/CARRG4MNC/p1628017550027100?thread_ts=1627502454.403900&cid=CARRG4MNC)

If accepted the intention is that the existing grade-school should be deprecated as the spec is too problematic.
I can add that as part of this PR if there is an agreed method?

[junedev]

@wolf99 In the JavaScript track someone suggested an additional (or changed) test case for this exercise: exercism/javascript#1337

Could you maybe have look whether that could be covered with your new system and whether you want to include that case?

[petertseng]

As of this time, here is what my review has covered:

• I see that the description.md, metadata.toml, and metadata.yml are exactly the same as the existing ones of grade-school.
• The proposed API given by add, roster, and grade are agreeable to me.
• The test cases' correctness, about which I have things to say. The verifier used is at https://github.com/petertseng/exercism-problem-specifications/blob/grade1837-ver/exercises/grade-school2/verify.rb and will be updated if any significant changes to the API are implemented.

Here are things I have no opinion on, and so review of that is left to others (I will not review these things):

• The mechanism by which grade-school should be replaced, if at all.

[petertseng]

Here is something that I think should be a necessary component of this: It should be clearly specified in the description that we are not allowing two students to have the same name.

Here are some other cases you might consider. I consider these optional; I would still approve a correct PR that does not contain these cases.

• add: Some case where there should be a false followed by a true - ensures that a failed addition doesn't prevent future successful additions.
• roster: Analogous to the above. Add Aimee, Aimee, Bethany, and ensure that Bethany is in the roster.
• grade: Analogous to the above.

[ErikSchierboom]

Reading #1824, the main problem mentioned was that the duplicate add behaviour was not well-specified. You also mention:

Intention after the mentioned PR was to rewrite the spec completely such that it covers the operation of adding students, as this clears up this and a lot of other issues.

What other issues are you referring to?

[wolf99]

Thanks for the comments all.

@petertseng I will update these now. The metadata etc are the same as grade-school as the intention is to specify the same problem, but in such a way as it can be solved without logical inconsistencies.

@junedev I will make up a commit to add this shortly

@ErikSchierboom I was being a bit enthusiastic there I think. Main thing is to resolve the logical inconsistency in #1763 and #1824. #1819 was intended to resolve #1763, but I felt it wasn't the best solution to the issue.

[ErikSchierboom]

I'm having a hard time figuring out what exactly changed :) The issue was with the add method, right? And the fix was to return a boolean indicating if the value could be added?

I have confirmed that the new JSON file has no problems with test cases correctness. If there are other considerations, they will be reviewed separately.

[wolf99]

Hi @ErikSchierboom ,
The issue for one of the existing add test cases was that I had specified that it expected true output, whereas going byt hte other add test cases I should have specified [true] (singuar vs array of one).
So the change for this comment was quite small.
It is on line 24 in the diff for that commit: e43e0b8

[wolf99]

My new commit just now should, I hope, add the test cases suggested by @petertseng and @junedev
However it seems like the schema does not allow digits in an exercise name, thus the schema validation is failing 😒
Do I switch to grade-school-two, or something else ?

[wolf99]

The review comment from @petertseng here: #1837 (comment) made me notice that this test case does not fully test what was intended.

In the test case referred to in the comment, the intention is to ensure that James is not added to two grades.
But grade only returns one grade. So this test actually only confirms that James is in the selected grade, it does not confirm that James ins not also in other grades. Not sure if there is a good way to specify a test that would confirm what is actually intended.

Any thoughts?

[petertseng]

So this test actually only confirms that James is in the selected grade, it does not confirm that James ins not also in other grades. Not sure if there is a good way to specify a test that would confirm what is actually intended.

Indeed, not with what we currently have. The way that fits in most naturally with the existing schema may simply be to have another case with the same inputs and different desiredGrade, right? Another way to do it would be to create a new property, but that seems a bit much, so the new test case seems like a more natural fit.

[ErikSchierboom]

With the risk of sounding dumb: was the old exercise unsalvageable? As in: couldn't we reimplement some test cases and add new ones with proper behavior?

[petertseng]

With the risk of sounding dumb: was the old exercise unsalvageable? As in: couldn't we reimplement some test cases and add new ones with proper behavior?

roster and grade semantics are compatible between the two.

Raw data supporting the above statement:

The verifier for grade-school2 gets the correct result on all[1] grade-school cases without modification to either the cases or the verifier.
The verifier for grade-school gets the correct result on all grade-school2 cases with only one modification to the verifier (must only show any name only once for a grade answer, instead of potentially having duplicate names) and the only modification to the cases being correcting the incorrect case pointed out in #1837 (review), which also needs to be corrected for the grade-school2 verifier to get the correct result anyway.

[1]: excludes the one reimplemented grade-school case

add cases are new, so certainly they don't present a compatibility problem.

Based on these above statements about add, roster, and grade, right now it doesn't seem to me that a new exercise is needed.
I would defer to a reasoned and well-informed judgment by interested individuals if new information comes up that would indicate it's needed.
I'm not particularly for or against, but the above are the suggestions I would make to the interested individuals who are, and I will continue to make new suggestions if new information comes to light.

[wolf99]

TBH, I find it very hard to keep track of whats going on in a canonical-data.json when there are more than one or two test cases have "reimplements", thus, where we have several here, I though it better/easier to start afresh.

However, if you think it better to continue in the original grade-school rather than create grade-school2 I'm happy enough to attempt to "merge" the two together once we are happy with the new test cases here.

[junedev]

I was under the impression the two version are not compatible. But if they are, then I would vote for salvaging the existing exercise even if that means just adding another "re-implements" thingy for all test cases. As far as I know there is no (semi-automated) way to signal to all tracks that have the old version "this exercise is deprecated, implement this new exercise instead" so the chances the tracks apply the fixes is probably higher when they can re-sync the existing exercise instead (even if they need to adjust the code a bit).

[ErikSchierboom]

Thanks for the hard work!

[petertseng]

items reviewed: All test cases are correct. API addition implied by add is sensible and agreeable. API changes (if any) implied by changes to roster and grade are not only sensible, they are compatible with previous. Wherever I looked at an individual case and could think of an additional case that could be added, I suggested it.

items I did not review (if someone else cares about them, that interested individual must review these items themselves): I mentioned I looked at individual cases and considered whether to add any cases. I did not look at the overall suite to see if there's a group of cases that might be missing. So I only looked at the trees and not the forest.

changes suggested: None are required. Two in-line items, one optional and one highly suggested. Two more highly suggested:

highly suggested: For a commit history that makes it easy to see what changed in this PR, please amend commits so that this PR ends up with two commits: one commit that reindents the existing file with no other changes, and one commit that applies all other changes (add cases, change descriptions). When merging, these commits must not be squashed. If you don't want to do this but do want me to do it, I will.

highly suggested: the description.md for this problem should describe what happens when a student is added twice. The existing note that "Note that all our students only have one name." does not cover this, since that sentence is saying that the students are only referred to by a given name and not a family name/surname.

appendix: the diff required of the verifier was relatively small.

diff --git a/exercises/grade-school/verify.rb b/exercises/grade-school/verify.rb
index 1a51d52f..b2c83ec6 100644
--- a/exercises/grade-school/verify.rb
+++ b/exercises/grade-school/verify.rb
@@ -1,9 +1,15 @@
 require 'json'
+require 'set'
 require_relative '../../verify'
 
 json = JSON.parse(File.read(File.join(__dir__, 'canonical-data.json')))
 
-cases = by_property(json['cases'], %w(roster grade))
+cases = by_property(json['cases'], %w(add roster grade))
+
+verify(cases['add'], property: 'add') { |i, _|
+  taken_names = Set.new
+  i['students'].map { |name, _| !!taken_names.add?(name) }
+}
 
 reimplemented, not_reimplemented = cases['roster'].partition { |c| c['uuid'] == 'c125dab7-2a53-492f-a99a-56ad511940d8' }
 
@@ -16,5 +22,5 @@ verify(not_reimplemented, property: 'roster') { |i, c|
 }
 
 verify(cases['grade'], property: 'grade') { |i, _|
-  (i['students'].group_by(&:last)[i['desiredGrade']] || []).map(&:first).sort
+  (i['students'].uniq(&:first).group_by(&:last)[i['desiredGrade']] || []).map(&:first).sort
 }

[petertseng]

I accept, as it fits one of the things I saw as a requirement: Must have one case that has an expected false followed by expected true.

optional: Might it seem like a smaller logical leap for the student to implement this if there were a case before this one that just adds James twice (expect true, false)? That way the expectation is isolated and hopefully isolated makes easy to understand.

[wolf99]

Thanks for the reviews!

I have added the missing test suggested by Peter and added another line to the description.md to cover the behavior for multiple students with the same name.

I have also split the indent correction to its own commit and squashed all other commits.

I have not added the additional optional test Peter has suggested for two reasons:

1. IIRC we do not try to have exhaustive tests for error conditions, and there are now quite a number of tests already in the exercise
2. I could not think of a good way to describe this test that does not conflict with the other test descriptions. This is important for clarity and for language tracks that create test names based on the test description strings

Neither of these are very strong reasons, if anyone has a good suggestion for 2, I could overlook 1.

[petertseng]

All changes I want have been done. I have nothing more to say.

[wolf99]

Just need one more approval here, perhaps @glennj or @junedev could oblige as you were both already interested in this ?

[glennj]

Yes, these tests now reflect the behaviour I would expect. Thank you.

[ErikSchierboom]

Thanks @wolf99!