-
-
Notifications
You must be signed in to change notification settings - Fork 511
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
Separate canonical data methods from ExerciseCase #648
Separate canonical data methods from ExerciseCase #648
Conversation
c640dfd
to
80aafb1
Compare
96eb2ce
to
2cb75d8
Compare
#650 has been merged, rebased to master and force-pushed this branch. |
They just cause everything to break unnecessarily. These tests will be added back in again at the end of the refactoring.
So we can access its properties using dot notation.
This reverts commit 56192974a5b79ed45b74f9bc00bb4a77b386e40a.
2cb75d8
to
e08ebb2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally like it, a couple of changes that can be made, but nothing really stopping it from coming in.
@@ -17,7 +17,7 @@ def beer_song_arguments | |||
if property == 'verse' | |||
number | |||
else | |||
"%s, %s" % [self["beginning"], self["end"]] | |||
"%s, %s" % [self.beginning, self.end] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm... beginning
and ending
would be a match here, and would avoid the 'stroop effect' caused by the standard use of begin
and end
in Ruby.
Something to consider. Not a required change for bringing this in though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These names come from the canonical data so we're constrained by what it provides.
@@ -19,7 +19,4 @@ def single_quote(string) | |||
string.inspect.tr('"', "'") | |||
end | |||
|
|||
def ignore_method_length | |||
"# rubocop:disable MethodLength\n " if board.length > 8 | |||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay! This is gone!
test/generator/exercise_case_test.rb
Outdated
@@ -3,45 +3,41 @@ | |||
module Generator | |||
class ExerciseCaseTest < Minitest::Test | |||
def test_name | |||
assert_equal 'test_foo', ExerciseCase.new(description: 'foo').name | |||
subject = ExerciseCase.new(canonical: OpenStruct.new('description' => 'foo')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the change in style from symbol: key
to 'string' => key
here, while maintaining the style of symbol: key
in the most of the rest of the file? (see line 34 as another deviation from the style)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair comment. Updated to use symbols.
(This was a left-over from the hash to OpenStruct conversion.)
@@ -16,7 +16,7 @@ def indent(size, lines) | |||
end | |||
|
|||
def assertion | |||
return error_assertion unless expected | |||
return error_assertion if expected == false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return error_assertion unless expected
Or
return error_assertion if expected.false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do the other cases address the assertions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is false?
defined?
Neither of these will work.
expected
comes from the canonical data and is either not present or an integer or false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That answers that then. 👍 Thanks.
from Generator Improvements #485.
Supersedes #640 (Closed)
Depends on #650 (Closed/Merged)
Completes #622 (Closed) to a usable state.