twelve-days: Implement canonical-data.json #954

alpakhomov · 2017-10-14T18:02:48Z

Resolves #590 // m-a-age

alpakhomov · 2017-10-14T18:08:12Z

I'm not sure about my implementation of canonical-data.json. The problem is that a lot of language tracks test every single line of the song - like 1, 2, 3 and so on till 12. I don't understand the reason behind it - it looks redundant to me. So, I think it's important to test the first and the last lines (checking the offset if some array-like data structure is used). Then, we check that all other parts of the song are there when we will run tests for intervals (for example, from first to third line). So, I don't really know what should be a priority - sticking to test cases that already are in many language tracks, or creating a suite of essential tests.
And maybe I'm wrong about this "test redundancy" in general.

Insti · 2017-10-14T20:35:35Z

With only 12 items it doesn't seem unreasonable to test every verse individually.

alpakhomov · 2017-10-15T14:56:02Z

Added test cases for every verse and changed their descriptions accordingly

Insti · 2017-10-15T18:27:38Z

exercises/twelve-days/canonical-data.json

+  "version": "1.0.0",
+  "cases": [
+    {
+      "description": "returns verse number 1",


I'd like to see more descriptive descriptions. This does not tell me anything that cannot be automatically generated from the input.

How about:
"first day a partridge in a pair tree"
"second day two turtle doves"
etc.

Or something similar.

Insti · 2017-10-15T18:29:39Z

exercises/twelve-days/canonical-data.json

+    {
+      "description": "finds multiple verses, which are a fragment of the song",
+      "property": "verses",
+      "input": [1, 3],


There is no test for verses that doesn't start at 1
what should happen in that case?

alpakhomov · 2017-10-15T19:11:55Z

Thanks @Insti, I've changed test descriptions and added test for interval from the middle of the song (4-6). Also changed interval test description accordingly.

I have also rebased commits into one.

petertseng

it seems to me that for the verse tests, instead of having "input": [1], it can just be "input": 1, - it will never be the case that the input should contain more than one verse number, so might as well not use an array.

input also seems unnecessary for sing.

petertseng

Nice.

I am wondering whether the trailing newlines on all expected outputs are necessary. Naively I might assume they aren't, but maybe there is something I don't know.

After this, I think I have said all I need to say. I haven't explicitly checked that all the expected values are correct, so I leave that to others (in other words, please don't wait for my approval before merging)

alpakhomov · 2017-10-15T23:18:58Z

Thank you @petertseng
About this newlines - I just copied them from current implementations, it looks like most people included them in their tests. But I don't know if there is any logic behind it.

ErikSchierboom · 2017-10-17T06:31:15Z

I am wondering whether the trailing newlines on all expected outputs are necessary. Naively I might assume they aren't, but maybe there is something I don't know.

I agree. It makes sense for the verses to be separated by newlines, but for an individual verse it doesn't IMHO. Maybe this is the point where we should remove those extra newlines?

alpakhomov · 2017-10-17T06:49:52Z

Okay, I a bit confused now, so, just to be clear - we are talking about last newline symbol at the end of each verse/block of verses, right?

Insti · 2017-10-17T07:11:09Z

exercises/twelve-days/canonical-data.json

+      "description": "first day a partridge in a pear tree",
+      "property": "verse",
+      "input": 1,
+      "expected": "On the first day of Christmas my true love gave to me, a Partridge in a Pear Tree.\n"


I believe the suggestion is that this line should not have the \n at the end.

Removed trailing newline for single verse tests

Insti · 2017-10-17T07:12:46Z

exercises/twelve-days/canonical-data.json

+      "description": "finds multiple verses, which are a fragment from the beggining of the song",
+      "property": "verses",
+      "input": [1, 3],
+      "expected": "On the first day of Christmas my true love gave to me, a Partridge in a Pear Tree.\n\nOn the second day of Christmas my true love gave to me, two Turtle Doves, and a Partridge in a Pear Tree.\n\nOn the third day of Christmas my true love gave to me, three French Hens, two Turtle Doves, and a Partridge in a Pear Tree.\n"


But whenever there are multiple verses they should be separated by \n\n
~~But no trailing newline.~~ Maybe the trailing single newline should be there.

But it only makes sense in the context of multiple verses, so why add it to a single verse?

Maybe the trailing single newline should be there.

In this case I am wondering about how many newlines should be at the end of the completed verses string. 0, 1 or 2? Currently there is 1.

I still agree there should be 0 at the end of each individual verse.
And that the joined verses should be separated by newlines.

I would expect the following to hold:

A single verse would have no newline at the end

Verses (more than one) are separated by an empty line

Verses don't have trailing newlines (so that means 0)

alpakhomov · 2017-10-17T07:52:44Z

A single verse would have no newline at the end
Verses (more than one) are separated by an empty line
Verses don't have trailing newlines (so that means 0)

Fixed

Insti

Looks great 👍

Insti · 2017-10-17T08:30:28Z

Do you have an automated checker for this exercise @petertseng ?

petertseng

Do you have an automated checker for this exercise @petertseng

Nope.

petertseng · 2017-10-17T09:31:19Z

exercises/twelve-days/canonical-data.json

+    {
+      "description": "finds multiple verses, which are a fragment from the beggining of the song",
+      "property": "verses",
+      "input": [1, 3],


hmm, sorry to swoop in with another thing again, but I just remembered... what about assigning meaning to the two elements of this array? See how beer song does it at https://github.com/exercism/problem-specifications/blob/master/exercises/beer-song/canonical-data.json#L54-L55 . Any desire to do the same here?

Added two elements instead of one array; named them beginning and end respectively. Not sure if it will be a good idea to split all cases into two groups, like "beer-song" does (for single and multiple verse test cases).

Should beginning and end be subkeys of input?

"input": {"beginning": 1, "end": 3}

Should it? Wouldn't it be confused with some object or map data structure?

Should beginning and end be subkeys of input?

That would be an interesting change to make for the JSON files all around. Then, instead of having to vary the input keys per exercise, test generators would know that all inputs can be found in "input". That could be a useful change for the schema! I think the track maintainer who wishes that to happen for all JSON files should make the issue requesting it.

rpottsoh · 2017-10-17T16:35:47Z

exercises/twelve-days/canonical-data.json

+      "end": 3,
+      "expected": "On the first day of Christmas my true love gave to me, a Partridge in a Pear Tree.\n\nOn the second day of Christmas my true love gave to me, two Turtle Doves, and a Partridge in a Pear Tree.\n\nOn the third day of Christmas my true love gave to me, three French Hens, two Turtle Doves, and a Partridge in a Pear Tree."
+    },
+


Line 84 should be removed.

rpottsoh · 2017-10-17T16:40:52Z

exercises/twelve-days/canonical-data.json

+    {
+      "description": "twelfth day twelve drummers drumming",
+      "property": "verse",
+      "input": 12,


Instead of "input" how about "number" since it is the verse number being asked for.

rpottsoh · 2017-10-17T16:41:10Z

If you would like to make this more beer-song like, you could group the Verse cases under a description of "Verse" and all the other cases under a description of "Lyrics".

alpakhomov · 2017-10-17T17:38:09Z

Thanks @rpottsoh
I've splitted cases into two groups (verse and lyrics), renamed input to number, and removed unnecessary empty line.

petertseng · 2017-10-17T21:54:56Z

Do you have an automated checker for this exercise

OK, guess that wasn't so much to write.

require 'json'
require_relative '../../verify'

DAYS = {
  twelfth: 'twelve Drummers Drumming',
  eleventh: 'eleven Pipers Piping',
  tenth: 'ten Lords-a-Leaping',
  ninth: 'nine Ladies Dancing',
  eighth: 'eight Maids-a-Milking',
  seventh: 'seven Swans-a-Swimming',
  sixth: 'six Geese-a-Laying',
  fifth: 'five Gold Rings',
  fourth: 'four Calling Birds',
  third: 'three French Hens',
  second: 'two Turtle Doves',
  first: 'a Partridge in a Pear Tree',
}.each_value(&:freeze).freeze

def list(things)
  things.size == 1 ? things[0] : (things[0...-1] << "and #{things[-1]}").join(', ')
end

def verse(n)
  ordinal = DAYS.keys[-n]
  gifts = DAYS.values.last(n)
  "On the #{ordinal} day of Christmas my true love gave to me, #{list(gifts)}."
end

def verses(a, b)
  (a..b).map { |n| verse(n) }.join("\n\n")
end

json = JSON.parse(File.read(File.join(__dir__, 'canonical-data.json')))

raise "There should be exactly two cases, not #{json['cases'].size}" if json['cases'].size != 2

verify(json['cases'][0]['cases'], property: 'verse') { |c|
  verse(c['number'])
}

subcases = by_property(json['cases'][1]['cases'], %w(verses sing))

verify(subcases['verses'], property: 'verses') { |c|
  verses(c['beginning'], c['end'])
}

verify(subcases['sing'], property: 'sing') { |c|
  verses(1, DAYS.size)
}

As written, it's fine.

Note that it may be a bit strange that both verses and sing are in the same group. As you can see though it can be dealt with.

Insti · 2017-10-18T12:38:20Z

Should verses and sing have their own sections?

Insti · 2017-10-18T12:40:42Z

Looking at the description and the policy that we should not be testing intermediate values it would seem that there should only be one test for the complete song.

(Sorry I've only just noticed this @alpakhomov )

The solution may be to update the description to add the requirement that verse ranges are also required, otherwise the tests will be passable by a hardcoded string, which seems like it would not be a "good" exercise.

The interface could then be, sing(x,y) where x <= y. and we don't need 3 different methods.

Thoughts?

alpakhomov · 2017-10-18T17:52:25Z

I changed properties and descriptions of tests in the lyrics block - now there is only one test for the whole song, plus we can still have only two groups of test cases (verse and lyrics). I think it looks better now - but I can revert chages back if needed.
About changes in exercise description - should it be done? If yes, do I need a separate pull request?

petertseng · 2017-10-23T08:31:18Z

If we take this PR as is, this exercise is on par with:

beer-song
- whose https://github.com/exercism/problem-specifications/blob/master/exercises/beer-song/description.md only says to "produce the lyrics"
- but whose https://github.com/exercism/problem-specifications/blob/master/exercises/beer-song/canonical-data.json checks individual verses
house
- whose https://github.com/exercism/problem-specifications/blob/master/exercises/house/description.md only says to "output the nursery rhyme"
- but whose https://github.com/exercism/problem-specifications/blob/master/exercises/house/canonical-data.json checks individual verses
food-chain
- whose https://github.com/exercism/problem-specifications/blob/master/exercises/food-chain/description.md only says to "generate the lyrics"
- but whose https://github.com/exercism/problem-specifications/blob/master/exercises/food-chain/canonical-data.json checks individual verses

Whatever treatment is given to this should also be given to all the others.

Insti · 2017-10-30T10:56:30Z

Hi @alpakhomov, are you willing to make some more changes to this to match what we're planning on doing to food-chain in #973

property: was verse and sing, both become recite

number,beginning, end: become start verse and end verse, each test case should have both. Where there was only one number they should start verse and end verse should be the same.

alpakhomov · 2017-10-30T11:19:49Z

Changed property and parameter names, everything else is the same

ErikSchierboom · 2017-10-30T11:24:07Z

Great work @alpakhomov ! What I was wondering, is if we should encoding the expected value as an array of strings instead of a single string, similar to what happens in the food-chain canonical data? My personal preference would be to use an array. @Insti, what are your thoughts?

Insti · 2017-10-30T11:35:26Z

encoding the expected value as an array of strings

I have no strong opinion, an array of strings seems fine, so long as there is a comment about it for the test implementer: https://github.com/exercism/problem-specifications/blob/master/exercises/food-chain/canonical-data.json#L4-L7

Insti

Convert the multi-line expectations into an array of strings.

ErikSchierboom · 2017-10-31T09:25:53Z

exercises/twelve-days/canonical-data.json

+        {
+          "description": "first day a partridge in a pear tree",
+          "property": "recite",
+          "start verse": 1,


We have now come up with an updated format, which requires the properties to be lowerCamelCase (see #983). Could you update the property to startVerse? And do the same for the other "start verse" properties?

ErikSchierboom · 2017-10-31T09:26:07Z

exercises/twelve-days/canonical-data.json

+          "description": "first day a partridge in a pear tree",
+          "property": "recite",
+          "start verse": 1,
+          "end verse": 1,


We have now come up with an updated format, which requires the properties to be lowerCamelCase (see #983). Could you update the property to endVerse? And do the same for the other "end verse" properties?

ErikSchierboom · 2017-10-31T09:27:10Z

exercises/twelve-days/canonical-data.json

+          "property": "recite",
+          "start verse": 2,
+          "end verse": 2,
+          "expected": "On the second day of Christmas my true love gave to me, two Turtle Doves, and a Partridge in a Pear Tree."


Could you encode the expected value as an array of strings? See #983 and #973. And do the same for the other "expected" properties?

ErikSchierboom · 2017-10-31T09:28:14Z

@alpakhomov Sorry about the churn in the changing requirements! It has taken us a little while to come up with uniform specifications, but the final result is in #983 and it should now be stable.

alpakhomov · 2017-10-31T11:24:32Z

Renamed parameters with camelCase, changed all expected values to arrays (even single line ones), added comment about expected values same as here https://github.com/exercism/problem-specifications/blob/master/exercises/food-chain/canonical-data.json#L4-L7

ErikSchierboom · 2017-11-02T07:41:38Z

I think this is ready to merge. @Insti, do you agree?

Insti · 2017-11-02T14:24:16Z

Yes it looks good to me.

ErikSchierboom · 2017-11-02T14:53:32Z

Merged! Thanks a lot @alpakhomov!

Insti · 2017-11-02T15:01:47Z

Thanks for all your work on this @alpakhomov and your patience with the constantly changing requirements ❤️

Insti reviewed Oct 15, 2017

View reviewed changes

alpakhomov force-pushed the twelve-days branch from 6f48f6a to e16ce00 Compare October 15, 2017 19:08

petertseng reviewed Oct 15, 2017

View reviewed changes

alpakhomov force-pushed the twelve-days branch from ac6f0ed to 2f85bb6 Compare October 15, 2017 23:14

petertseng reviewed Oct 15, 2017

View reviewed changes

Insti reviewed Oct 17, 2017

View reviewed changes

alpakhomov force-pushed the twelve-days branch 2 times, most recently from e5ada1f to e0a528e Compare October 17, 2017 07:49

ErikSchierboom approved these changes Oct 17, 2017

View reviewed changes

Insti approved these changes Oct 17, 2017

View reviewed changes

petertseng reviewed Oct 17, 2017

View reviewed changes

alpakhomov force-pushed the twelve-days branch from 4f41e70 to 3675be6 Compare October 17, 2017 10:42

rpottsoh reviewed Oct 17, 2017

View reviewed changes

alpakhomov force-pushed the twelve-days branch from 3675be6 to cf30929 Compare October 17, 2017 17:33

rpottsoh approved these changes Oct 17, 2017

View reviewed changes

alpakhomov force-pushed the twelve-days branch from cf30929 to 336aaf7 Compare October 18, 2017 17:51

Insti mentioned this pull request Oct 26, 2017

food-chain: Make properties less ambiguous #973

Merged

alpakhomov force-pushed the twelve-days branch from 336aaf7 to 6f995b2 Compare October 30, 2017 11:13

Insti mentioned this pull request Oct 30, 2017

recitation exercises: Standardise canonical data interface. #983

Closed

5 tasks

Insti suggested changes Oct 31, 2017

View reviewed changes

ErikSchierboom requested changes Oct 31, 2017

View reviewed changes

twelve-days: Implement canonical-data.json

201be91

alpakhomov force-pushed the twelve-days branch from 6f995b2 to 201be91 Compare October 31, 2017 11:14

ErikSchierboom approved these changes Oct 31, 2017

View reviewed changes

Insti approved these changes Oct 31, 2017

View reviewed changes

Insti mentioned this pull request Oct 31, 2017

beer-song: Update to test property recite #984

Merged

ErikSchierboom merged commit 7c59aaa into exercism:master Nov 2, 2017

twelve-days: Implement canonical-data.json #954

twelve-days: Implement canonical-data.json #954

Conversation

alpakhomov commented Oct 14, 2017 • edited by ilya-khadykin Loading

alpakhomov commented Oct 14, 2017

Insti commented Oct 14, 2017

alpakhomov commented Oct 15, 2017

Insti Oct 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpakhomov commented Oct 15, 2017

petertseng left a comment

Choose a reason for hiding this comment

petertseng left a comment

Choose a reason for hiding this comment

alpakhomov commented Oct 15, 2017

ErikSchierboom commented Oct 17, 2017

alpakhomov commented Oct 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Insti Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Insti Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

ErikSchierboom Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

alpakhomov commented Oct 17, 2017

Insti left a comment

Choose a reason for hiding this comment

Insti commented Oct 17, 2017

petertseng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Insti Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertseng Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpottsoh commented Oct 17, 2017

alpakhomov commented Oct 17, 2017

petertseng commented Oct 17, 2017

Insti commented Oct 18, 2017

Insti commented Oct 18, 2017 • edited Loading

alpakhomov commented Oct 18, 2017

petertseng commented Oct 23, 2017

Insti commented Oct 30, 2017

alpakhomov commented Oct 30, 2017

ErikSchierboom commented Oct 30, 2017

Insti commented Oct 30, 2017

Insti left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikSchierboom commented Oct 31, 2017

alpakhomov commented Oct 31, 2017

ErikSchierboom commented Nov 2, 2017

Insti commented Nov 2, 2017

ErikSchierboom commented Nov 2, 2017

Insti commented Nov 2, 2017

alpakhomov commented Oct 14, 2017 •

edited by ilya-khadykin

Loading

Insti Oct 15, 2017 •

edited

Loading

Insti Oct 17, 2017 •

edited

Loading

Insti Oct 17, 2017 •

edited

Loading

ErikSchierboom Oct 17, 2017 •

edited

Loading

Insti Oct 17, 2017 •

edited

Loading

petertseng Oct 17, 2017 •

edited

Loading

Insti commented Oct 18, 2017 •

edited

Loading

Insti left a comment •

edited

Loading