Isogram Exercise improvements: change method name/add failure messages by alexdebugs · Pull Request #664 · exercism/ruby

alexdebugs · 2017-05-31T19:29:18Z

Why?

Improves the Isogram Exercise by adding failure messages and simplifying the method name
Helps bring the exercise up to standard

What?

Added failure messages to tell you what value was expected and what input was tested
Shortened the method name from is_isogram? to isogram?
Changed from version 3 to version #4

How Has This Been Tested?

By generating the test for this exercise by running `bin/generate -f isogram' and then
testing the generated test.

Screenshots (if appropriate):

Types of changes

n/a Bug fix (non-breaking change which fixes an issue)
Enhancement (non-breaking change which adds functionality)
Breaking change (fix or enhancement that would cause existing functionality to change)
n/a Gardening (code and/or documentation cleanup)

Picked breaking change too because the method name changed so if you went back and ran the
version 3 solutions against this version they would fail.

References and Closures

references #475

Checklist:

I have read the CONTRIBUTING document.
My change relies on a pending issue/pull request
n/a My change requires a change to the documentation.
n/a I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

…sages

Insti · 2017-05-31T19:33:27Z

    # skip
    string = ""
-    assert Isogram.is_isogram?(string)
+    assert Isogram.isogram?(string), "Expected 'true', '' is an isogram"


I am not sure if true/false should be quoted. I have a slight preference for not quoting it.
@kotp?

We don't want people to think they have to return a string.

No definitely, I get what you're saying! I guess I was just going off the triangle exercise test case more or less. If we decide to change it on here I can also go back and change it here later https://github.com/exercism/xruby/blob/master/exercises/triangle/triangle_test.rb. 😄

Boolean should not be quoted.

Insti · 2017-05-31T19:34:32Z

Looks good 👍
I want to wait for @kotp to look this over, he has a good eye for this sort of thing.

Insti · 2017-05-31T19:36:11Z

+  end
+
+  def type
+    "#{expected ? 'is' : 'is not'} an isogram"


I like how you've left the 'is' in both cases. 👍

def failure_message %Q("Expected '#{expected}', '#{input}' #{reason}") end def reason "#{is_or_not} an isogram" end def is_or_not expected ? 'is' : 'is not' end

Since we aren't really getting a type, the method name is not quite right.

EDIT: I left input as it is, but perhaps that is result instead. input always seems too generic, and often is not expressing what it is well enough.

I like reason but think the input is part of the reason.
%Q("Expected '#{expected}', #{reason}")

For me it is a little bit easier to read it as Q("Expected '#{expected}', #{reason}") rather than %Q("Expected '#{expected}', '#{input}' #{reason}") if say I was just skimming through it.

Comment on @kotp's edit: input is the input - the string that is provided as input to the method under test.

Nothing we should do regarding 'input' for sure. I was just re-reading the code as you were writing the response.

kotp

Some considerations to be made.

kotp · 2017-05-31T23:36:02Z

+  end
+
+  def type
+    "#{expected ? 'is' : 'is not'} an isogram"


def failure_message %Q("Expected '#{expected}', '#{input}' #{reason}") end def reason "#{is_or_not} an isogram" end def is_or_not expected ? 'is' : 'is not' end

Since we aren't really getting a type, the method name is not quite right.

EDIT: I left input as it is, but perhaps that is result instead. input always seems too generic, and often is not expressing what it is well enough.

kotp · 2017-05-31T23:37:04Z

    # skip
    string = ""
-    assert Isogram.is_isogram?(string)
+    assert Isogram.isogram?(string), "Expected 'true', '' is an isogram"


Boolean should not be quoted.

kotp · 2017-05-31T23:37:57Z

  def test_bookkeeping
    skip
-    assert_equal 3, BookKeeping::VERSION
+    assert_equal 4, BookKeeping::VERSION


If messages are the only thing being changed, and not the tests themselves, then VERSION should not change.

Ah yes, removed the query part that is common in languages where the question mark cannot be used. So it is a good version bump.

alexdebugs · 2017-06-01T03:44:53Z

Wow thank you for all the feedback @kotp ! When I was making the method name I was actually wondering what to call it, so I'm happy for your insight.

alexdebugs · 2017-06-02T18:43:50Z

Submitted another commit and integrated your great feedback.
Let me know if you have any other feedback 😄

Insti · 2017-06-03T12:38:40Z

Thanks @deohtee, I will look at this when I get a chance.

kotp

Looks good to me.

Insti · 2017-06-07T06:13:46Z

Thanks @deohtee ❤️

Insti · 2017-06-08T09:34:15Z

Why didn't Travis run on these changes?

kotp · 2017-06-08T13:03:17Z

It looks like a missed hook communication.

Insti · 2017-06-09T06:32:33Z

Travis CI had some issues between May 31 and June 2 it looks like this PR came in during that window and the PR was never picked up by Travis.
Ref: https://www.traviscistatus.com/#month (link will go out of date with time.)

Isogram Exercise improvements: change method name and add failure mes…

307a3ff

…sages

Insti reviewed May 31, 2017

View reviewed changes

kotp reviewed May 31, 2017

View reviewed changes

Unquote booleans and integrate feedback for failure_message method

4951eed

kotp approved these changes Jun 4, 2017

View reviewed changes

Insti merged commit 63ee9f5 into exercism:master Jun 7, 2017

Insti removed the ready label Jun 8, 2017

Insti mentioned this pull request Jun 8, 2017

isogram: update solution to pass current tests. #669

Merged

Uh oh!

Conversation

alexdebugs commented May 31, 2017 • edited by Insti Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why?

What?

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

References and Closures

Checklist:

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 commented May 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kotp May 31, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kotp left a comment

Choose a reason for hiding this comment

Uh oh!

kotp May 31, 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!

Choose a reason for hiding this comment

Uh oh!

alexdebugs commented Jun 1, 2017

Uh oh!

alexdebugs commented Jun 2, 2017

Uh oh!

Insti commented Jun 3, 2017

Uh oh!

kotp left a comment

Choose a reason for hiding this comment

Uh oh!

Insti commented Jun 7, 2017

Uh oh!

Insti commented Jun 8, 2017

Uh oh!

kotp commented Jun 8, 2017

Uh oh!

Insti commented Jun 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alexdebugs commented May 31, 2017 •

edited by Insti

Loading

kotp May 31, 2017 •

edited

Loading

kotp May 31, 2017 •

edited

Loading