Skip to content

Implement generator for beer-song #553

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 11, 2017
Merged

Implement generator for beer-song #553

merged 1 commit into from
Mar 11, 2017

Conversation

kytrinyx
Copy link
Member

@kytrinyx kytrinyx commented Mar 9, 2017

I've submitted a pull request to update the canonical data for the beer-song exercise (exercism/problem-specifications#661). In order to make it easier to pull in those changes once the PR has been merged, I've created a generator for the beer-song exercise.

In addition to implementing the logic for the generator, I ran the bin/generate beer-song command to create a fresh implementation.

The structure of the canonical data is a bit different from the implementation that the Ruby track had:

Before:

BeerSong.new.verse(n)
BeerSong.new.verses(n, m)
BeerSong.new.lyrics

After:

BeerSong.new.verse(n)
BeerSong.new.verses(n, m)

In the old version, lyrics was a simple indirection, calling verses with the full range.

This is a breaking change, and the book-keeping has been updated to reflect this.

As I implemented the generator, I discovered that the documentation was outdated. This has been updated in #552

@kotp
Copy link
Member

kotp commented Mar 9, 2017

This may need to be updated after the exercism/problem-specifications#661 is brought in.

@kytrinyx kytrinyx force-pushed the beer-song-generator branch from 68065c6 to 0214000 Compare March 11, 2017 02:45
@kytrinyx
Copy link
Member Author

I've updated this per the newest canonical data.

"Take one down and pass it around, 98 bottles of beer on the wall.\n"
def test_first_generic_verse
# skip
expected = <<-TEXT
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the <<-TEXT means that we can have the ending delimiter indented.

At the moment the ending delimiter is on the 1 column, so this is not needed. See comment regarding the generator for note.

<%= test_case.skipped %>
expected = <<-TEXT
<%= test_case.expected %>
TEXT
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of the <<-TEXT we could indent the ending delimiter to the level of line 11.

  def <%= test_case.test_name %>
    <%= test_case.skipped %>
    expected = <<-TEXT
<%= test_case.expected %>
    TEXT
    assert_equal expected, <%= test_case.workload %>
  end
<% end %>

The other choice would be to remove the minus before the delimiter and leave the ending delimiter as it is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. Yepp. I don't have strong feelings either way, but I think I'll indent the ending delimiter.

Copy link
Member

@kotp kotp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. We could bring it in now and there would be no foreseen problems.

One small change in the template file is possible, either removing the heredoc delimiter prefix or using that prefix and indenting the ending delimiter.

@kytrinyx kytrinyx force-pushed the beer-song-generator branch from 0214000 to c0afeb9 Compare March 11, 2017 16:43
@kytrinyx
Copy link
Member Author

I've updated the indentation of the closing token for the heredoc.

I'm using the property to decide the name of the function, so as long as that doesn't change over in our canonical data schema discussion, I think we're good here.

@kotp kotp merged commit 1239f84 into master Mar 11, 2017
@behrtam
Copy link

behrtam commented Mar 16, 2017

I'm just curious ... wouldn't it make more sense now that the canonical test data has a real version (beer-song/canonical-data.json#L3) to use that instead of the sha-1 hash of the commit (# Common test data version: 9f3d48a)?

@kotp
Copy link
Member

kotp commented Mar 16, 2017

I think the merge was done before the canonical was finished... but I think using that canonical version now makes sense to reference.

@kytrinyx kytrinyx deleted the beer-song-generator branch March 17, 2017 01:39
@kytrinyx
Copy link
Member Author

Yeah, the canonical version makes much more sense. I didn't think of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants