proverb: Add canonical data. by coriolinus · Pull Request #994 · exercism/problem-specifications

coriolinus · 2017-11-03T01:52:32Z

This is based on the Rust tests as implemented in exercism/rust#380.
The lack of canonical data had resulted in odd custom tests being
designed, which was reported in exercism/rust#379.

Closes #578.

This is based on the Rust tests as implemented in exercism/rust#380. The lack of canonical data had resulted in odd custom tests being designed, which was reported in exercism/rust#379.

Insti · 2017-11-03T08:02:08Z

A couple of things that came up during #983 might be appropriate here:

Name the method recite
The expected output should be an array of strings without newlines.
Make sure the exercise and version properties are at the top.

See the canonical-data.json for food-chain as an example.

Insti · 2017-11-03T08:03:05Z

+                    "property": "construct"
+                },
+                {
+                    "description": "three pieces modernized",


There seem to be four pieces in the array.

Insti · 2017-11-03T08:04:20Z

It's useful to link to the description: https://github.com/exercism/problem-specifications/blob/master/exercises/proverb/description.md

Insti · 2017-11-03T08:09:08Z

+                {
+                    "description": "full proverb",
+                    "expected": "For want of a nail the shoe was lost.\nFor want of a shoe the horse was lost.\nFor want of a horse the rider was lost.\nFor want of a rider the message was lost.\nFor want of a message the battle was lost.\nFor want of a battle the kingdom was lost.\nAnd all for the want of a nail.",
+                    "input": [


This method of input is not really specified in the description and is an implementation detail.

We either need to:

Update the description to specify that you will be provided with a list of items.

Change the tests so we're just testing for the specified verses. (see also food-chain.)

I think in order to differentiate this exercise from food-chain it might be good to use the "list of items" version.

cc: @ErikSchierboom

@Insti I agree that having a different input here makes for a more interesting exercise. The existing exercises are quite similar, so having some variety is nice.

So the description should also be updated to clarify the problem.

- name the method 'recite' - expect output as an array of strings without newlines - move exercise, version properties to the top - rename 'three pieces modernized' to 'four pieces modernized', for accuracy - add comment regarding output format - add link to problem description

- Eliminate bogus word 'horseshoe' from last line of example proverb - Add text noting that the inputs may vary

coriolinus · 2017-11-03T17:17:53Z

@Insti Good ideas all; I've incorporated all of them.

petertseng · 2017-11-03T23:22:18Z

+            "cases": [
+                {
+                    "description": "zero pieces",
+                    "expected": [


I was considering to ask that this should have zero elements, but given that this is converted to a string by joining all elements with newlines, then [""] and [] are both representations of the empty string, so do whatever you want.

petertseng · 2017-11-04T00:15:15Z

+                    "property": "recite"
+                }
+            ],
+            "description": "Construct a proverb of the form 'for want of an X, the Y was lost'"


I wonder if someone will miss this at the crucial time because it's on the bottom. But I don't think it's necessary at all. I would remove it and remove one level of cases nesting.

The description doesn't say that the lines of the proverb could vary, only the "input".

- Remove empty string from output - Un-nest cases - Move inner cases description to problemset comment - Amplify inner cases description - Re-order case fields for more sensible reading

rpottsoh · 2017-11-04T15:43:19Z

+                "For want of a pin the gun was lost.",
+                "For want of a gun the soldier was lost.",
+                "For want of a soldier the battle was lost.",
+                "And all for the want of a pin."


Is it to be presumed that the proverb is not set in stone? The description only refers to a single proverb and makes not mention that the "input" provided might refer to a different proverb. I don't think "four pieces modernized" should be included unless the description is also updated.

@coriolinus my apologies! I didn't read closely enough. ~~You are passing the expected proverb.~~

ugh, I just need to slow down. My original concern still stands. With the "input" provided how are we to know that it is for a different proverb when only one proverb is described in the description.

@rpottsoh I did update the description for that: "your solution should be able to handle lists of arbitrary length and content".

In short, the idea is to ensure that students won't just hard-code the entire poem as a list, and then return its last N items, where N is the length of the input list. The goal of the exercise is to ensure that they can manipulate strings.

This also helps distinguish this exercise from others; discussion here was in favor of keeping the input list.

Thanks @coriolinus for clearing this up for me. I have not worked this exercise yet so some of the what was getting past me. Thanks for taking the time to produce the canonical data for this exercise.

wolf99 · 2017-11-04T21:04:41Z

+        "lines together with line breaks. ",
+        "All cases expect the student to construct a proverb of the form ",
+        "'For want of an X, the Y was lost.', terminating with the stock phrase ",
+        "'And all for the want of a Z.'"


Looks like Z is a special case for one of the inputs - could be worth specifically calling that out in the description.md

No? I don't understand your comment; all inputs of non-zero length terminate with that stock phrase.

The description.md doesn't specifically draw any explicit connection between the last line and the first input. This especially is confusing if you are aware of the proverb IRL, where the inputs are always the same and the last line is always "And all for the want of a nail".

Going solely by the description, it seems like Z should always be "nail" but that is not the case. Yes, the data implicitly makes this a requirement in a single case - but surely the data should be informed by the requirement (i.e. the description) rather than it introducing a requirement by itself?

What I mean to say is that by the description it is obvious that the inputs affect X and Y, but it is not obvious that they affect Z; Z is really X[0] so to speak.

I should have added this note against the description.md rather than this line I realise 😅

I see now what you mean, though I'm not sure that I agree: the obvious terminating phrase, to me, would always be f"And all for the want of a {input[0]}.". Still, I don't mind adding a bit of text to the description to make that more clear.

The description.md specifies the problem so I think you can just remove these 3 lines (8-10) from the config.json.

rpottsoh

Thanks @coriolinus!

Insti · 2017-11-05T17:13:44Z

Thanks @coriolinus ❤️

Actually, the canonical data was born from the Rust implementation. exercism/problem-specifications#994 1.1.0 moves inputs to the `input` object. exercism/problem-specifications#1157 The Rust track has all the canonical tests, although in a different order. Reviewers should indicate whether an ordering change in the Rust tests is desired.

Add canonical data to the proverb exercise.

dc3b61b

This is based on the Rust tests as implemented in exercism/rust#380. The lack of canonical data had resulted in odd custom tests being designed, which was reported in exercism/rust#379.

Insti reviewed Nov 3, 2017

View reviewed changes

Insti changed the title ~~Add canonical data to the proverb exercise.~~ proverb: Add canonical data. Nov 3, 2017

coriolinus added 2 commits November 3, 2017 18:11

Update description to describe inputs

e39146e

- Eliminate bogus word 'horseshoe' from last line of example proverb - Add text noting that the inputs may vary

remove 'description' field, which broke Travis

215c0f5

petertseng reviewed Nov 3, 2017

View reviewed changes

petertseng reviewed Nov 4, 2017

View reviewed changes

coriolinus mentioned this pull request Nov 4, 2017

READMEs: Import problem-specifications commits exercism/rust#354

Merged

More updates to canonical-data.json

fc75a99

- Remove empty string from output - Un-nest cases - Move inner cases description to problemset comment - Amplify inner cases description - Re-order case fields for more sensible reading

rpottsoh reviewed Nov 4, 2017

View reviewed changes

wolf99 reviewed Nov 4, 2017

View reviewed changes

coriolinus and others added 2 commits November 5, 2017 15:27

Clarify that all output lines vary according to the input

33fcffe

Remove redundant comment from canonical-data.json

a0af54d

rpottsoh approved these changes Nov 5, 2017

View reviewed changes

Insti approved these changes Nov 5, 2017

View reviewed changes

coriolinus merged commit b15c45a into master Nov 5, 2017

coriolinus deleted the add-proverb-canonical-data branch November 5, 2017 17:12

Insti mentioned this pull request Nov 6, 2017

recitation exercises: Standardise canonical data interface. #983

Closed

5 tasks

petertseng mentioned this pull request Nov 18, 2018

proverb: no-op declare compliance with 1.1.0 exercism/rust#738

Merged

Uh oh!

Conversation

coriolinus commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Insti commented Nov 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Insti commented Nov 3, 2017

Uh oh!

Insti Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coriolinus commented Nov 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpottsoh Nov 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpottsoh Nov 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wolf99 Nov 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wolf99 Nov 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpottsoh left a comment

Choose a reason for hiding this comment

Uh oh!

Insti commented Nov 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

coriolinus commented Nov 3, 2017 •

edited

Loading

Insti Nov 3, 2017 •

edited

Loading

rpottsoh Nov 4, 2017 •

edited

Loading

rpottsoh Nov 4, 2017 •

edited

Loading

wolf99 Nov 4, 2017 •

edited

Loading

wolf99 Nov 5, 2017 •

edited

Loading