Add anagram generator by fredrb · Pull Request #464 · exercism/ruby

fredrb · 2016-10-18T02:39:06Z

#396 - Anagram exercise generator

Insti · 2016-10-18T05:53:06Z

-    detector = Anagram.new('diaper')
-    assert_equal [], detector.match(%w(hello world zombies pants))
+     # skip
+     assert_equal [], Anagram.new("diaper").match(["hello", "world", "zombies", "pants"])


indentation should be 2 spaces, you've got 3 here.

Insti · 2016-10-18T06:00:13Z

+  end
+
+  def work_load
+    actual = "Anagram.new(\"#{subject}\").match(#{candidates})"


If you use single quotes around #{subject} you don't need to escape the quotes.

Insti · 2016-10-18T06:04:34Z

+  end
+
+  def show_comment
+    comment.nil? ? '' : "# #{comment}\n\t\t"


\t is the wrong character to be using.
Ideally there would be no formatting characters in this file.
Perhaps you can find a solution that leaves the formatting in the template file and the logic in this file?

I was enforcing this because I didn't want a blank line in the generated file if there are no comments for the test case. But it makes sense that no formatting should be done here.

There are other ways to achieve the same effect.

Insti · 2016-10-18T06:05:11Z

+  def test_detects_unicode_anagrams
+     skip
+     # These words don't make sense, they're just greek letters cobbled together.
+		assert_equal ["ΒΓΑ", "γβα"], Anagram.new("ΑΒΓ").match(["ΒΓΑ", "ΒΓΔ", "γβα"])


Use spaces rather than tabs for indentation.

Insti · 2016-10-18T06:07:13Z

-    assert_equal [], detector.match(['eagle'])
+  def test_detects_multiple_anagrams
+     skip
+     assert_equal ["stream", "maters"], Anagram.new("master").match(["stream", "pigeon", "maters"])


You're enforcing an ordering here which is not specified in the readme.
['matters','stream'] would also be a valid result.

Insti · 2016-10-18T06:08:26Z

-    assert_equal ['tan'], anagrams
+  def test_detects_simple_anagram
+     skip
+     assert_equal ["tan"], Anagram.new("ant").match(["tan", "stand", "at"])


Why have you changed from the old 3 line "detector, anagrams, assertion" format?

Insti · 2016-10-18T06:10:18Z

-    assert_equal [], detector.match(['last'])
+  def test_detects_anagram
+     skip
+     assert_equal ["inlets"], Anagram.new("listen").match(["enlists", "google", "inlets", "banana"])


This is a personal preference of mine and not something that needs changing if you have a different opinion, but I'd rather see single quotes around strings that don't require interpolation.

Agreed. But both the expected and matched lists come from the canonical json file.

Insti · 2016-10-18T06:13:42Z

Travis CI is failing because the example solution does not pass all the tests:

  1) Failure:
AnagramTest#test_detects_unicode_anagrams [/tmp/anagram.oOJIjvdSpC/anagram_test.rb:87]:
Expected: ["ΒΓΑ", "γβα"]
  Actual: ["ΒΓΑ"]

Insti · 2016-10-18T06:19:34Z

Thanks @fredrb ❤️ It's great to see people contributing new generators.
You've covered all the important functionality and the things to change are are mostly just minor stylistic and consistency issues.

Great work.

Insti · 2016-10-18T07:50:06Z

Expected: ["ΒΓΑ", "γβα"]
Actual: ["ΒΓΑ"]

I see you've raised an issue about this in x-common.
That was the right thing to do.

fredrb · 2016-10-18T10:44:54Z

Thanks for the review @Insti. I fixed most of the things you pointed out and left a comment on others. I'll check the issue with the greek letters in x-common then, if it gets fixed, I'll re-generate the files here.

Insti · 2016-10-18T11:25:28Z

-     assert_equal ["gallery", "regally", "largely"], Anagram.new("allergy").match(["gallery", "ballerina", "regally", "clergy", "largely", "leading"])
+    skip
+
+    assert_equal ["gallery", "regally", "largely"].sort, Anagram.new('allergy').match(["gallery", "ballerina", "regally", "clergy", "largely", "leading"]).sort


You didn't address my other question:

Why have you changed from the old 3 line "detector, anagrams, assertion" format?

That would help avoid situations like this where you end up with one really long line.

Insti · 2016-10-18T11:26:49Z


  def show_comment
-    comment.nil? ? '' : "# #{comment}\n\t\t"
+    comment.nil? ? '' : "# #{comment}"


Try moving the comment outputing into the work_load, this should allow you to avoid empty lines.

Insti · 2016-10-18T11:46:29Z

+  def work_load
+    actual = "Anagram.new('#{subject}').match(#{candidates})"
+    "assert_equal #{expected}.sort, #{actual}.sort"
+  end


A suggestion:

def work_load expectation = "expected = #{expected}" actual = "actual = Anagram.new('#{subject}').match(#{candidates})" assertion = "assert_equal expected.sort, actual.sort" indent_lines( [ show_comment, expectation, actual, assertion ].compact ) end def indent_lines(lines, indent_level = 2) lines.join("\n" + ' '*2*indent_level) end

Although the formatting of the indent_lines method call could be improved.

fredrb · 2016-10-18T22:41:22Z

@Insti, I implemented the changes you suggested, could you please review again?
Our tests are failing because exercism/problem-specifications#414 is still pending.

Insti · 2016-10-19T05:31:48Z

+
+  private
+
+  def ident_lines(code, ident = 2)


"indent" is missing an 'n'

Insti

This is really nice.

I think the multi-line work_load makes it much clearer what is going on.

I really like what you've done in anagram_cases using small methods and simplifying the whole thing.

Great work.

Insti · 2016-10-19T05:42:59Z

exercism/problem-specifications#414 has now been merged.

Insti · 2016-10-19T05:49:23Z

-    anagrams = detector.match(%w(tan stand at))
-    assert_equal ['tan'], anagrams
+    anagrams = detector.match(["tan", "stand", "at"])
+    assert_equal ["tan"].sort, anagrams.sort


(This is an 'in an ideal world' type issue please don't think that you have to do this.)

Based on the principle that we want to make the test cases as nice as possible for people and would prefer generator complexity over test case complexity.
It could be nice to omit the .sort when there is only one element, and pre-sort the 'expected' arrays that have multiple elements.

Insti · 2016-10-19T05:52:42Z

+  end
+
+  def assert
+    "assert_equal #{expected}.sort, anagrams.sort"


"ideal world" suggestion: (see my other comment)

assert_equal #{expected.sort}, anagrams#{expected.size > 1 ? '.sort' : ''}

or something more elegant.

Insti · 2016-10-19T07:00:50Z

+
+# Test data version:
+# <%= sha1 %>
+class AnagramTest < Minitest::Test<% test_cases.each do |test_case| %>


Can we move the <% .. %> down to the next line?

fredrb · 2016-10-19T11:44:29Z

Good idea omiting the .sort when expected list has either a single value or it's empty. If you check the test class, only two test cases ended up with anagrams.sort 👍

Insti · 2016-10-19T17:47:22Z

I think this is ready to go now, I'll just wait a bit and give @kotp a chance to look over it before merging.

Thanks for your work on this and your responsiveness to feedback, I think we've ended up with a really nice generator AND nice test cases. Great work. ❤️

fredrb · 2016-10-19T18:15:46Z

@Insti It was a pleasure! Thanks for the code reviews and suggestions

kotp · 2016-10-19T21:53:38Z

@fredrb can you squash these commits into one or (if you think it makes sense) 2 commits. It makes it easier to review, and gives structure to the commit message when it comes in, as to what you would like communicate in the log.

I end up doing it locally, to review, but it is nice to give you a chance to cultivate the commit, as I may miss something.

kotp

The changes all together look really good.

There are trailing whitespace that ends up in the generated test file on or around line 9. We would want to not do that.

fredrb · 2016-10-19T23:55:38Z

Squashed them into one, @kotp. Also made the small fix for the trailing whitespace.

kotp · 2016-10-20T01:07:37Z

Thank you @fredrb

fredrb mentioned this pull request Oct 18, 2016

Anagram test data exercism/problem-specifications#413

Closed

Insti reviewed Oct 18, 2016

View reviewed changes

Insti reviewed Oct 19, 2016

View reviewed changes

Insti approved these changes Oct 19, 2016

View reviewed changes

Insti reviewed Oct 19, 2016

View reviewed changes

Insti added the in progress label Oct 19, 2016

Insti reviewed Oct 19, 2016

View reviewed changes

Insti mentioned this pull request Oct 19, 2016

Replace hard-coded Bowling tests with test generator (Fixes #456) #465

Merged

Insti approved these changes Oct 19, 2016

View reviewed changes

kotp approved these changes Oct 19, 2016

View reviewed changes

Add anagram test generator

af734a7

fredrb force-pushed the anagram_generator branch from 4604fe3 to af734a7 Compare October 19, 2016 23:54

kotp merged commit f646982 into exercism:master Oct 20, 2016

Insti removed the in progress label May 19, 2017

Insti removed the ready label Jun 8, 2017

Uh oh!

Conversation

fredrb commented Oct 18, 2016

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!

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!

Insti Oct 18, 2016 • 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!

Insti Oct 18, 2016 • 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!

Insti commented Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Insti commented Oct 18, 2016

Uh oh!

Insti commented Oct 18, 2016

Uh oh!

fredrb commented Oct 18, 2016

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!

fredrb commented Oct 18, 2016

Uh oh!

Insti Oct 19, 2016 • 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!

Insti left a comment

Choose a reason for hiding this comment

Uh oh!

Insti commented Oct 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Insti Oct 19, 2016 • 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!

fredrb commented Oct 19, 2016

Uh oh!

Insti commented Oct 19, 2016

Uh oh!

fredrb commented Oct 19, 2016

Uh oh!

kotp commented Oct 19, 2016

Uh oh!

kotp left a comment • 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!

fredrb commented Oct 19, 2016

Uh oh!

kotp commented Oct 20, 2016

Uh oh!

Reviewers

Insti Oct 18, 2016 •

edited

Loading

Insti Oct 18, 2016 •

edited

Loading

Insti commented Oct 18, 2016 •

edited

Loading

Insti Oct 19, 2016 •

edited

Loading

Insti commented Oct 19, 2016 •

edited

Loading

Insti Oct 19, 2016 •

edited

Loading

kotp left a comment •

edited

Loading