Skip to content

Carry #549: Add generator for etl#575

Merged
kotp merged 4 commits intoexercism:masterfrom
hilary:add-generator-for-etl
Apr 28, 2017
Merged

Carry #549: Add generator for etl#575
kotp merged 4 commits intoexercism:masterfrom
hilary:add-generator-for-etl

Conversation

@hilary
Copy link
Copy Markdown
Contributor

@hilary hilary commented Apr 28, 2017

Description

Carry @ajwann's valuable work forward so we can get it in :)

See original PR for description, motivation and context.

How Has This Been Tested?

rake test

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@hilary hilary mentioned this pull request Apr 28, 2017
3 tasks
@hilary hilary requested a review from kotp April 28, 2017 00:59
Comment thread lib/etl_cases.rb Outdated

def indent(size, text)
text.lines.each_with_object('') { |line, obj| obj << ' ' * size + line }
end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ExerciseCases needs some magic indenting code in it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It has some magic indenting code, indent_lines.

I've started converting #workload methods over to using it when I have a reason to rewrite a #workload method.

Copy link
Copy Markdown
Contributor

@tommyschaefer tommyschaefer left a comment

Choose a reason for hiding this comment

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

I noticed a small inconsistency between the static and generated tests. This does not cause the tests to fail because they are technically correct, as both the old and expected hashes use strings.

Comment thread exercises/etl/etl_test.rb Outdated
def test_a_single_letter
# skip
old = {
'1' => ["A"],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like the generated test use strings rather than numbers ('1' vs 1)

Comment thread exercises/etl/etl_test.rb Outdated
'1' => ["A"],
}
expected = {
'a' => '1',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as above, except for the value rather than the key

Comment thread lib/etl_cases.rb Outdated
def format_hash(hash)
middle = hash.each_pair.with_object('') do |(k, v), obj|
value = v.class == Array ? v : "'#{v}'"
obj << " '#{k}' => #{value},\n "
Copy link
Copy Markdown
Contributor

@tommyschaefer tommyschaefer Apr 28, 2017

Choose a reason for hiding this comment

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

When given a hash with integerized keys from here, the key is still wrapped in quotes, negating #integerize_keys.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

good spot @tommyschaefer 👍

@hilary
Copy link
Copy Markdown
Contributor Author

hilary commented Apr 28, 2017

Nice catch on the keys, @tommyschaefer . I'll get a chance to look at that after I get some sleep :)

Copy link
Copy Markdown
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 is good to go, I think.

@kotp kotp merged commit 9a73ca5 into exercism:master Apr 28, 2017
@hilary hilary mentioned this pull request Apr 30, 2017
3 tasks
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.

5 participants