etl: added generator by ajwann · Pull Request #549 · exercism/ruby

ajwann · 2017-02-19T19:27:30Z

Description

Created a generator for the etl exercise. The generated test is functionally identical to the original, and the formatting of the hashes is as close as I could get it.

Motivation and Context

Part of Epic #396

How Has This Been Tested?

Generator has been ran and test file was successfully generated.

Screenshots (if appropriate):

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)~~

References and Closures

References #396

Checklist:

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

bin/local-status-check returns no failures
rubocop -D returns no failures

Admin edit: Using strikethrough rather than empty checkboxes, so it doesn't look like an incomplete todo list.

ajwann · 2017-02-19T19:31:28Z

+  def test_single_score_with_multiple_letters
    skip
-    old = { 1 => %w(A E I O U) }
-    expected = { 'a' => 1, 'e' => 1, 'i' => 1, 'o' => 1, 'u' => 1 }


The previous test was arbitrarily placing key/value pairs on newlines. I decided it was easiest to just check if there was more than one key/value pair in a hash and if so, print each key value pair on a separate line.

ajwann · 2017-02-19T19:34:45Z

-      4 => %w(F H V W Y),
-      5 => %w(K),
-      8 => %w(J X),
-      10 => %w(Q Z)


The previous test was also arbitrarily using the shorthand %w() syntax for arrays. Personally I think it's best if all tests just use one syntax or another, so I went with the standard bracket syntax. I can definitely switch everything to the %w() syntax though if that's the consensus.

Personally I think it's best if all tests just use one syntax or another

Yes! Agreed 👍

I can definitely switch everything to the %w() syntax though if that's the consensus.

Personally, I think the longer examples are a bit easier to read when using %w(), but I'm not sure if everyone else will agree with that. This is likely a question of preference, so a weigh in from @Insti and @kotp would be great. I don't think its much of a big deal though 😄

Unsure it was 'arbitrary' as it does save some typing... like 2/3rds for each single character "word", after the first element. For a single element entry, it is identical in character length. It was probably a convenience of typing, and something that is idiomatic (even encouraged in the style guide).

Consistency is good though.

Personally I think that new Ruby users coming from other languages would be able to recognize and understand the bracket syntax faster than the %w() syntax. But that's just my opinion and I'm happy to change the generator to use only the %w() syntax.

tommyschaefer

Thank you so much for the PR!

There are a few things that could use some updating, but overall this looks like it's moving in the right direction!

tommyschaefer · 2017-02-19T19:47:30Z

+  private
+
+  def indent(size, lines)
+    lines.lines.each_with_object('') { |line, obj| obj << ' ' * size + line }


lines.lines looks a bit strange to me.

If you need to call lines on the input, it that suggests to me that it's not actually lines and a different name might be preferable. One that we've used in the past is text

Agreed, this looks really strange. I'll fix it.

tommyschaefer · 2017-02-19T20:11:30Z

+    input.reduce({}) { |hash, (k, v)| hash[k.to_i] = v; hash }
+  end
+
+  def format_hash(hash)


I think this method can be simplified a bit.

Try operating on the hash as a hash, rather than as a string.

I created this method to format the hash neatly across multiple lines; adding newlines and indentation. Without it, each hash is printed on one line, and those lines are really long. I'm not sure how I can achieve the same result without converting it to a string.

@ajwann I think @tommyschaefer is hinting at something like:

middle = hash.each_pair.with_object('') do |(k, v), obj| obj << " :#{k} => #{v},\n " end " {\n#{middle}}"

kotp · 2017-02-20T22:11:16Z

@tommyschaefer Can you pull this locally, and run a bundle exec generate -f etl and see if exercises/etl/etl_test.rb changes in the test_bookkeeping method as far as version goes?

This seems to be unique here, as it does not seem to do the same in Hamming, for example. It freezes like it should.

ajwann · 2017-04-22T20:48:10Z

@kotp @tommyschaefer; sorry to disappear for so long, it's been a busy month. I operated on the hash using each_with_object as @kotp suggested, and it definitely made life a lot easier.

Complied with, but the comment was on lines above the code needing to be changed.

kotp · 2017-04-23T04:53:42Z

@ajwann It looks like your x-common is now out of date. The generator will not build this on the current canonical data. Can you confirm and fix?

kotp · 2017-04-27T23:33:30Z

@hilary Can you review this, and let us know what needs to be done here?

hilary · 2017-04-27T23:45:33Z

@kotp will do!

hilary · 2017-04-28T00:12:58Z

Minimal required work before merging:

first and most important, this branch needs to be rebased onto master.
Remove the EtlCases loop from lib/etl_cases.rb

hilary · 2017-04-28T00:58:53Z

PR carried forward in #575

hilary · 2017-04-28T01:06:26Z

Closing this PR now that we've carried the work forward. Let's get this generator in :)

@ajwann I've been working on reducing the effort to add a generator. When you recover from this one and get a chance to take a swing at another, I would love to hear whether you think the changes have actually reduced the effort or not.

Carry #549: Add generator for etl

ajwann · 2017-04-30T15:04:27Z

Thank you @hilary! I'll be starting on another generator soon, I'll let you know how it goes.

I'm curious as to what improvements have been made; can you point me to an issue or PR that describes the changes?

hilary · 2017-04-30T19:05:46Z

@ajwann I think the best way to see what is needed to write a generator is to look at the xruby README, which we're keeping up to date.

If you'd like to see what we're planning, get involved in the discussion, etc., we have an issue for the generator changes: #485

ajwann commented Feb 19, 2017

View reviewed changes

tommyschaefer previously requested changes Feb 19, 2017

View reviewed changes

etl: added generator

b046aab

hilary closed this Apr 28, 2017

kotp added a commit that referenced this pull request Apr 28, 2017

Merge pull request #575 from hilary/add-generator-for-etl

9a73ca5

Carry #549: Add generator for etl

Uh oh!

Conversation

ajwann commented Feb 19, 2017 • edited by Insti Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

References and Closures

Checklist:

Uh oh!

ajwann Feb 19, 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!

ajwann Feb 19, 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!

tommyschaefer Feb 19, 2017

Choose a reason for hiding this comment

Uh oh!

kotp Feb 20, 2017

Choose a reason for hiding this comment

Uh oh!

ajwann Feb 20, 2017

Choose a reason for hiding this comment

Uh oh!

tommyschaefer left a comment

Choose a reason for hiding this comment

Uh oh!

tommyschaefer Feb 19, 2017

Choose a reason for hiding this comment

Uh oh!

ajwann Feb 20, 2017

Choose a reason for hiding this comment

Uh oh!

tommyschaefer Feb 19, 2017

Choose a reason for hiding this comment

Uh oh!

ajwann Feb 20, 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!

kotp Feb 20, 2017

Choose a reason for hiding this comment

Uh oh!

kotp commented Feb 20, 2017

Uh oh!

ajwann commented Apr 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kotp commented Apr 23, 2017

Uh oh!

kotp commented Apr 27, 2017

Uh oh!

hilary commented Apr 27, 2017

Uh oh!

hilary commented Apr 28, 2017

Uh oh!

hilary commented Apr 28, 2017

Uh oh!

hilary commented Apr 28, 2017

Uh oh!

ajwann commented Apr 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hilary commented Apr 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ajwann commented Feb 19, 2017 •

edited by Insti

Loading

ajwann Feb 19, 2017 •

edited

Loading

ajwann Feb 19, 2017 •

edited

Loading

ajwann Feb 20, 2017 •

edited

Loading

ajwann commented Apr 22, 2017 •

edited

Loading

ajwann commented Apr 30, 2017 •

edited

Loading