beer-song: Update to test property recite #984

robkeim · 2017-10-30T13:55:16Z

Addresses #983

Note:

The end verse value is numerically less than the start verse for multi-verse tests which may seem counter-intuitive but makes sense given the song counts down from 99 to 0

robkeim · 2017-10-30T14:01:40Z

After re-reading the issue I need to update to "start verse" and "end verse" instead of "startVerse" and "endVerse"

rpottsoh

I believe the verse number has nothing to do with the number of bottles of bear on the wall. There are 99 bottles of beer on the wall at the beginning of the song. A song begins at its first verse.

Verses are ascending while bottles of beer on the wall is descending.

Insti · 2017-10-30T14:47:39Z

Would start bottles and end bottles make more sense for this exercise?

There are some things to think about here.

rpottsoh · 2017-10-30T15:07:32Z

Would start bottles and end bottles make more sense for this exercise?

I think that it would. I reviewed my solution to this problem. (I should have done that before I submitted my review.)

My implementation passes in the number of bottles and not the verse number, and thus effectively passes in start bottles and end bottles.

I will update my review of this PR.

@Insti

@Insti's suggestion of using start bottles and end bottles in my opinion fixes the issue I had.

robkeim · 2017-10-30T15:15:27Z

start bottles and end bottles would make more sense although it's not consistent with the other three verse exercises then. It's probably a better approach than having to do math to figure out which verse corresponds to how many bottles.

rpottsoh · 2017-10-30T15:16:22Z

exercises/beer-song/canonical-data.json

-              "number": 99,
+              "property": "recite",
+              "start verse": 99,
+              "end verse": 99,


Consider startBottles and endBottles instead. Same for all occurrences later in this files.

rpottsoh · 2017-10-30T15:17:30Z

exercises/beer-song/description.md

@@ -1,4 +1,4 @@
-Produce the lyrics to that beloved classic, that field-trip favorite: 99 Bottles of Beer on the Wall.


lyrics was OK.

#983 calls for standardizing the descriptions as well as the property name which is why I made this change.

#983 calls for standardizing the descriptions as well as the property name

Hmm I didn't notice that :)
I like lyrics better than verses here tbh.

robkeim · 2017-10-30T15:18:41Z

@Insti and @rpottsoh updated to startBottles and endBottles.

rpottsoh · 2017-10-30T15:23:52Z

exercises/beer-song/canonical-data.json

-              "start verse": 3,
-              "end verse": 3,
+              "startBottles": 3,
+              "endBottles": 3,
              "expected": "3 bottles of beer on the wall, 3 bottles of beer.\nTake one down and pass it around, 2 bottles of beer on the wall.\n"
            },
            {
              "description": "verse 2",


Since we are not strictly talking verses anymore, perhaps this should be:

"description": "2 bottles of beer",

or something to the affect.

Similar changes would be needed for the other cases.

I'd prefer to change this to verse with 2 bottles because we have first generic verse and last generic verse which are more explicit than 99 bottles of beer and 3 bottles of beer in terms of why those test cases were chosen. Thoughts?

I like it. Thanks.

rpottsoh

Thanks @robkeim

Insti · 2017-10-30T15:45:27Z

exercises/beer-song/description.md

@@ -1,4 +1,4 @@
-Produce the lyrics to that beloved classic, that field-trip favorite: 99 Bottles of Beer on the Wall.
+Recite the verses to that beloved classic, that field-trip favorite: 99 Bottles of Beer on the Wall.


The change to Recite is good.

"Recite the lyrics" is better (and more consistent) than "Recite the verses"

Insti · 2017-10-30T15:51:41Z

exercises/beer-song/canonical-data.json

-              "number": 99,
+              "property": "recite",
+              "startBottles": 99,
+              "endBottles": 99,


There are 98 bottles left on the wall at the end...

Insti · 2017-10-30T15:53:32Z

exercises/beer-song/canonical-data.json

+              "description": "verse with 0 bottles",
+              "property": "recite",
+              "startBottles": 0,
+              "endBottles": 0,


There are 99 bottles on the wall at the end.

Maybe startBottles and endBottles doesn't make sense for this problem.

startBottles and takeDowns? perhaps.

(Trying to work out what's best here, not advocating changes just yet.)

takeBottles? bottlesToTake?

Whatever it is we're diverging from the common specification, but I think that is OK as this is a separate exercise with its own nuances.

For the first two verses are you suggesting:

"startBottles": 99, "takeDowns": 2,

or

"startBottles": 99, "takeDowns": 98,

The first makes more sense to me but that's moving away in consistency with the other exercises unfortunately. If we're going to head down that direction, I think numVerses would be more explicit.

Woops... just read your other comments you're one step ahead of me :)

Should the words in the lyrics dictate that naming of the properties?

When we get down to zero beers on the wall, the only reason there ends up being 99 bottles of beer on the wall is because the lyrics say to go buy more beer.

To trigger a trip to the store, endBottles has to be zero.

I'm actually suggesting:

"startBottles": 99, "takeDowns": 1,

rpottsoh · 2017-10-30T15:57:33Z

Grabbing lunch.... you bring up a good point. TakeDown would be less disruptive than endBottles. Sent from my android device.

rpottsoh · 2017-10-30T16:36:12Z

TakeDown would be less disruptive than endBottles.

I have to correct myself. The above is not a true statement. The way the exercise is implemented is the start number of bottles and the resulting number of bottles are passed in as parameters. So I say stick with startBottles and endBottles.

Insti · 2017-10-30T18:44:18Z

The way the exercise is implemented is the start number of bottles and the resulting number of bottles are passed in as parameters. So I say stick with startBottles and endBottles.

That is an unnecessary implementation detail.

The test needs to specify, the number of bottles that begin on the wall startBottles and the number of verses to output. There are various ways to specify this, but using the language of the song seems like the most appropriate, therefore take n (down).

This also works in the zero wrap around case where we can start with 0 and take 1 down and end at 99 again. If we were to take more than 1 we could still expect the right answer.

If you REALLY wanted to maintain consistency (which you shouldn't) the thing to do would be to update the other exercises to take startVerses and numberOfVerses parameters.
(But don't do this.)

In this case breaking consistency with the way the other exercises are implemented is a good thing.

rpottsoh · 2017-10-30T18:58:15Z

@Insti I am coming around to your way of seeing this exercise. These changes accentuate the perpetual nature behind this song..... It never ends. 😄

Insti · 2017-10-30T19:09:41Z

With takeDown one could also specify a test that was

"startBottles": 99,
"takeDown": 200,

And ensure that it looped through twice. This would not be possible with the start/end pair.

(I don't recommend adding this test either.)

robkeim · 2017-10-31T12:14:00Z

@Insti and @rpottsoh I've updated to takeDown. Please double check I didn't make a silly off-by-one error :)

rpottsoh

All the takeDowns look fine to me.

Insti · 2017-10-31T13:51:58Z

Looks good. 👍
One last thing..
In food-chain, house and twelve-days we return an array of strings rather than a single newline separated string. Can you add another commit that converts the expected values to that format please?

Insti · 2017-11-02T18:55:05Z

@robkeim, Just waiting on this one now. If you don't have time we can merge this PR and do the line adjustments as a new PR.

robkeim · 2017-11-03T12:36:46Z

@Insti I should be able to get to this on Sunday. You can either merge this as is or wait for that update.

robkeim · 2017-11-03T20:30:39Z

@Insti I couldn't sleep so I took advantage of the extra time to wrap-up this PR :) It should be ready for you to merge.

rpottsoh

Looks good, thanks @robkeim

Insti · 2017-11-05T16:08:46Z

Thanks for all your work on this @robkeim ❤️

robkeim · 2017-11-05T16:11:14Z

You're welcome @Insti, thanks for the feedback!

Update beer-song to use recite

6b320c0

Fix casing in names

a42ea11

robkeim mentioned this pull request Oct 30, 2017

recitation exercises: Standardise canonical data interface. #983

Closed

5 tasks

rpottsoh previously requested changes Oct 30, 2017

View reviewed changes

Insti changed the title ~~Update beer-song to test property recite~~ beer-song: Update to test property recite Oct 30, 2017

rpottsoh reviewed Oct 30, 2017

View reviewed changes

Renamed to startBottles and endBottles

c6bcebf

rpottsoh reviewed Oct 30, 2017

View reviewed changes

Update descriptions to be more explicit

78252b8

rpottsoh approved these changes Oct 30, 2017

View reviewed changes

Insti suggested changes Oct 30, 2017

View reviewed changes

Change to recite the lyrics

7eed603

Insti reviewed Oct 30, 2017

View reviewed changes

Use takeDown instead of endBottles

c332bec

rpottsoh approved these changes Oct 31, 2017

View reviewed changes

Multiline format for expected result

df9c28d

rpottsoh approved these changes Nov 3, 2017

View reviewed changes

ErikSchierboom approved these changes Nov 5, 2017

View reviewed changes

Insti approved these changes Nov 5, 2017

View reviewed changes

Insti merged commit ddb52e6 into exercism:master Nov 5, 2017

robkeim deleted the recite-canonical-data branch November 5, 2017 16:10

		@@ -1,4 +1,4 @@
		Produce the lyrics to that beloved classic, that field-trip favorite: 99 Bottles of Beer on the Wall.

		@@ -1,4 +1,4 @@
		Produce the lyrics to that beloved classic, that field-trip favorite: 99 Bottles of Beer on the Wall.
		Recite the verses to that beloved classic, that field-trip favorite: 99 Bottles of Beer on the Wall.

beer-song: Update to test property recite #984

beer-song: Update to test property recite #984

Conversation

robkeim commented Oct 30, 2017

robkeim commented Oct 30, 2017

rpottsoh left a comment

Choose a reason for hiding this comment

Insti commented Oct 30, 2017 • edited Loading

rpottsoh commented Oct 30, 2017

robkeim commented Oct 30, 2017

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

robkeim commented Oct 30, 2017

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

rpottsoh left a comment

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

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

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

rpottsoh commented Oct 30, 2017 via email

rpottsoh commented Oct 30, 2017

Insti commented Oct 30, 2017

rpottsoh commented Oct 30, 2017

Insti commented Oct 30, 2017

robkeim commented Oct 31, 2017

rpottsoh left a comment

Choose a reason for hiding this comment

Insti commented Oct 31, 2017 • edited Loading

Insti commented Nov 2, 2017

robkeim commented Nov 3, 2017

robkeim commented Nov 3, 2017

rpottsoh left a comment

Choose a reason for hiding this comment

Insti commented Nov 5, 2017

robkeim commented Nov 5, 2017

Insti commented Oct 30, 2017 •

edited

Loading

Insti commented Oct 31, 2017 •

edited

Loading