space-age: Add generator by ajwann · Pull Request #583 · exercism/ruby

ajwann · 2017-04-30T19:07:30Z

Why?

space-age exercise needs a generator

What?

added generator for space-age

How Has This Been Tested?

rake test:space-age
rubocop -D

Screenshots (if appropriate):

Types of changes

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

References and Closures

references #396

Checklist:

I have read the CONTRIBUTING document.
n/a 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.
n/a I have added tests to cover my changes.
All new and existing tests passed.

ajwann · 2017-04-30T19:10:44Z

-  def test_age_in_seconds
-    age = SpaceAge.new(1_000_000)
-    assert_in_delta 1_000_000, age.seconds, DELTA
-  end


This method was missing from x-common, so I omitted it. If it's needed, I can add it to x-common or manually hack it in before the test_age_on_earth method in space_age_cases.rb

Leaving it out is fine. If it's not specified in the readme or the canonical data it probably shouldn't be there.

tommyschaefer · 2017-04-30T20:50:19Z

Hi There! First off, thank you for your help converting this exercise to use generated tests!

There has been some really awesome new work on generators by @hilary that has changed how we implement new generators. Check out the updated documentation, for some tips on upgrading this to the new pattern!

Thank you again for your work and time! 😄

hilary · 2017-04-30T22:46:43Z

You might also find it helpful to see what the final version of the etl PR looked like: #575.

ajwann · 2017-05-01T02:00:53Z

Awesome, thank you both. I've read over the final etl PR and I'll take a look at the new docs as soon as I can this week.

ajwann · 2017-05-02T00:01:50Z

+      "age = SpaceAge.new(#{underscore_format(seconds)})",
+      indent(4, assertion),
+    ].join("\n")
+  end


Now workload is the only public method as per the docs.

ajwann · 2017-05-02T00:02:26Z

+
+# Common test data version: <%= abbreviated_commit_hash %>
+class <%= exercise_name_camel %>Test < Minitest::Test
+  DELTA = 0.01


I still needed a custom template to add this constant.

I've created an issue for this: #589

I wonder if a comment explaining what delta is used for would be helpful for people who are meeting assert_in_delta for the first time.

I think that would be helpful, I'll add it.

I̶'̶v̶e̶ ̶t̶h̶o̶u̶g̶h̶t̶ ̶a̶b̶o̶u̶t̶ ̶w̶h̶a̶t̶ ̶t̶h̶i̶s̶ ̶c̶o̶m̶m̶e̶n̶t̶ ̶s̶h̶o̶u̶l̶d̶ ̶s̶a̶y̶,̶ ̶a̶n̶d̶ ̶I̶'̶m̶ ̶k̶i̶n̶d̶ ̶o̶f̶ ̶e̶m̶b̶a̶r̶r̶a̶s̶s̶e̶d̶ ̶t̶o̶ ̶s̶a̶y̶ ̶t̶h̶a̶t̶ ̶I̶ ̶d̶o̶n̶'̶t̶ ̶r̶e̶a̶l̶l̶y̶ ̶k̶n̶o̶w̶ ̶w̶h̶a̶t̶ ̶d̶e̶l̶t̶a̶ ̶m̶e̶a̶n̶s̶ ̶i̶n̶ ̶t̶h̶i̶s̶ ̶c̶a̶s̶e̶.̶ ̶B̶e̶s̶i̶d̶e̶s̶ ̶t̶h̶e̶ ̶b̶a̶s̶i̶c̶ ̶m̶a̶t̶h̶e̶m̶a̶t̶i̶c̶s̶ ̶d̶e̶f̶i̶n̶i̶t̶i̶o̶n̶ ̶o̶f̶ ̶"̶c̶h̶a̶n̶g̶e̶ ̶i̶n̶ ̶a̶ ̶g̶i̶v̶e̶n̶ ̶q̶u̶a̶n̶t̶i̶t̶y̶"̶,̶ ̶I̶'̶m̶ ̶k̶i̶n̶d̶ ̶o̶f̶ ̶s̶t̶u̶m̶p̶e̶d̶.̶

Thanks Internet. I'll add a comment.

ajwann · 2017-05-02T00:04:23Z

I think I've moved everything where it belongs, please let me know if I missed anything.

Insti · 2017-05-02T05:18:03Z

+  def test_age_on_mercury
    skip
    age = SpaceAge.new(2_134_835_688)
    assert_in_delta 67.65, age.on_earth, DELTA


I don't like that the on_earth method is also being tested here, I wouldn't do it in real life code, but I cannot tell if this test has educational value and should remain.

https://github.com/exercism/xruby#workload-philosophy

Thoughts?

I don't think it provides a lot value, other than to give the user a baseline for which to compare the age difference. I personally think we could take out the on_earth assertions for each test and just leave the single test_age_on_earth test.

Insti · 2017-05-02T05:28:20Z

+  def workload
+    [
+      "age = SpaceAge.new(#{underscore_format(seconds)})",
+      indent(4, assertion),


There are some (currently undocumented) methods in ExerciseCases that can do ~~underscore formatting and~~ indenting for you.

https://github.com/exercism/xruby/blob/master/lib/generator/exercise_cases.rb

We should probably also add Integer underscore formatting to https://github.com/exercism/xruby/blob/master/lib/generator/underscore.rb

Sounds good; I can use the indent_lines method of ExerciseCases, and add the integer underscore formatting method I'm using to the Underscore module.

Insti

I like what you've done here and would be happy to merge it as is, but have made some comments I'd be interested in hearing your thoughts on.

Insti · 2017-05-02T05:41:42Z

Sorry for all the complications here @ajwann, you're on the 🔪 bleeding edge 🔪 of test generation right now!

ajwann · 2017-05-11T22:56:39Z

I think we are finally good to go!

Insti · 2017-05-12T05:52:47Z

@@ -0,0 +1,26 @@
+#!/usr/bin/env ruby
+gem 'minitest', '>= 5.0.0'


This line is no longer used.

Insti · 2017-05-12T05:57:07Z

+
+    refine Fixnum do
+      def underscore
+        self.to_s.reverse.gsub(/...(?=.)/, '\&_').reverse


This is good 👍

Insti · 2017-05-12T05:57:56Z

@@ -0,0 +1,31 @@
+class SpaceAgeCase < Generator::ExerciseCase
+  using Generator::Underscore


Is Generator::Underscore already used by Generator::ExerciseCase?

Yep, it definitely is. I can swap my using statement in favor of requiring 'generator/exercise_case'.

It turns out you can not, due to lexical scope issues. Sorry for misleading you.

http://blog.honeybadger.io/understanding-ruby-refinements-and-lexical-scope/

Insti · 2017-05-12T05:59:27Z

This is looking good @ajwann, sorry we keep moving the goalposts on you!

Insti · 2017-05-12T06:11:48Z

 require 'minitest/autorun'
 require_relative 'space_age'

+# Common test data version: 7c63e40


This line is missing the canonical_data_version

See: https://github.com/exercism/xruby/blob/master/lib/generator/test_template.erb

These comments should be about the test_template.erb version. (sorry)

Insti · 2017-05-12T06:18:50Z

@@ -0,0 +1,31 @@
+class SpaceAgeCase < Generator::ExerciseCase


There should be a require 'generator/exercise_case' at the top of this file.

See: https://github.com/exercism/xruby#implementing-a-generator

Insti · 2017-05-12T06:19:37Z

+  def assertion
+    [
+      "    #{assertion_for_location(expected, planet)}",
+    ]


Does this need to be an array?

Insti · 2017-05-12T06:20:13Z

+
+end
+
+SpaceAgeCases = proc do |data|


This section is no longer needed.

Insti · 2017-05-12T06:23:13Z

+      "age = SpaceAge.new(#{seconds.underscore})",
+      assertion,
+    ].join("\n")
+  end


There might be a method in Generator::ExerciseCase that can join and indent for you.

I ended up using indent_lines.

Insti · 2017-05-12T06:25:16Z

 class SpaceAgeTest < Minitest::Test
+  # assert_in_delta will pass if the difference
+  # between the values being compared is less
+  # than the allowed delta


Great explanatory comment. 👍

ajwann · 2017-05-14T22:54:47Z

@Insti I've made all the requested updates, but I am having trouble testing the changes. I rebased off of master, but when I attempt to run bin/generate space-age I get an error saying that a generator currently doesn't exist for spage-age. My generator is still in exercises/space-age/.meta/generator/ so I'm not sure what's causing the issue.

Insti · 2017-05-15T09:22:09Z

@ajwann Thanks, I'll have a look into this for you when I get a chance.

Insti · 2017-05-15T09:58:01Z

@@ -1,67 +1,82 @@
 #!/usr/bin/env ruby
+gem 'minitest', '>= 5.0.0'


~~This line is no longer required/used and can be deleted.~~
Oh, you have, you've just not regenerated the tests due the problem you were having. No problem.

Insti · 2017-05-15T10:11:13Z

@ajwann

when I attempt to run bin/generate space-age I get an error saying that a generator currently doesn't exist for spage-age. My generator is still in exercises/space-age/.meta/generator/ so I'm not sure what's causing the issue.

The expected filename has changed, it should now be called space_age_case.rb (no 's' on 'case'). More apologies for changing everything on you!

Insti · 2017-05-15T10:32:24Z

+
+  def workload
+    indent_lines(["age = SpaceAge.new(#{seconds.underscore})",
+      "    assert_in_delta #{expected}, age.on_#{planet.downcase}, DELTA"


The spaces between " and assert are not required when you're using indent_lines.

ajwann · 2017-05-18T23:05:35Z

+require 'generator/exercise_case'
+
+class SpaceAgeCase < Generator::ExerciseCase
+  using Generator::Underscore


I'm not too familiar with refinements, but it looks like this using statement is necessary.
It doesn't look like the using statement in ExerciseCase gets inherited by SpaceAgeCase.

Yeah, I also discovered this and commented here: #583 (comment)

It turns out you can not, due to lexical scope issues. Sorry for misleading you.
http://blog.honeybadger.io/understanding-ruby-refinements-and-lexical-scope/

Looks like github managed to hide the comment.

I also made an issue about addressing this here in xruby: #645

But including it for now is good. 👍

Insti · 2017-05-19T05:55:24Z

+
+class SpaceAgeCase < Generator::ExerciseCase
+  using Generator::Underscore
+  SECONDS_PER_YEAR_ON_EARTH = 31557600


Is this constant used/necessary?

ajwann · 2017-05-19T19:44:24Z

+
+class SpaceAgeCase < Generator::ExerciseCase
+  using Generator::Underscore
+


@Insti; I definitely didn't need that constant, it was left over from when each assertion was unnecessarily testing the age on Earth.

Insti · 2017-05-19T19:51:16Z

Awesome! Thanks for all your work on this @ajwann. I've merged it now, so any more changes we make to the process will be our responsibility to fix! 😄

ajwann commented Apr 30, 2017

View reviewed changes

Insti changed the title ~~space-age: added generator~~ [WIP] space-age: added generator May 1, 2017

Insti changed the title ~~[WIP] space-age: added generator~~ [WIP] space-age: Add generator May 1, 2017

kotp added the in progress label May 1, 2017

ajwann force-pushed the add-generator-for-space-age branch 2 times, most recently from f0e7283 to ec3fce6 Compare May 2, 2017 00:00

ajwann commented May 2, 2017

View reviewed changes

Insti reviewed May 2, 2017

View reviewed changes

ajwann force-pushed the add-generator-for-space-age branch 2 times, most recently from 823be7e to 515686c Compare May 11, 2017 22:55

Insti reviewed May 12, 2017

View reviewed changes

ajwann force-pushed the add-generator-for-space-age branch from 515686c to 4a91b62 Compare May 14, 2017 22:24

ajwann force-pushed the add-generator-for-space-age branch from 4a91b62 to e4615ad Compare May 14, 2017 22:50

Insti reviewed May 15, 2017

View reviewed changes

Insti mentioned this pull request May 15, 2017

generator: more helpful error message when not found. #644

Closed

Insti reviewed May 15, 2017

View reviewed changes

This was referenced May 15, 2017

generator: refinements and lexical scope. #645

Closed

Be more helpful when generator can not be found #646

Merged

ajwann force-pushed the add-generator-for-space-age branch from e4615ad to db627f8 Compare May 18, 2017 22:56

ajwann commented May 18, 2017

View reviewed changes

Insti reviewed May 19, 2017

View reviewed changes

space-age: added generator

f11ad7b

ajwann force-pushed the add-generator-for-space-age branch from db627f8 to f11ad7b Compare May 19, 2017 19:33

ajwann commented May 19, 2017

View reviewed changes

Insti changed the title ~~[WIP] space-age: Add generator~~ space-age: Add generator May 19, 2017

Insti merged commit f421126 into exercism:master May 19, 2017

Insti removed the in progress label May 19, 2017

tommyschaefer mentioned this pull request May 19, 2017

Exercise tests should be generated #396

Closed

94 tasks

Insti mentioned this pull request May 21, 2017

Test Fixnum#underscore #652

Merged

Insti removed the ready label Jun 8, 2017

		@@ -0,0 +1,26 @@
		#!/usr/bin/env ruby
		gem 'minitest', '>= 5.0.0'

		@@ -0,0 +1,31 @@
		class SpaceAgeCase < Generator::ExerciseCase
		using Generator::Underscore

		@@ -1,67 +1,82 @@
		#!/usr/bin/env ruby
		gem 'minitest', '>= 5.0.0'

Uh oh!

Conversation

ajwann commented Apr 30, 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!

ajwann Apr 30, 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!

tommyschaefer commented Apr 30, 2017

Uh oh!

hilary commented Apr 30, 2017

Uh oh!

ajwann commented May 1, 2017

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!

ajwann May 11, 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 commented May 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajwann May 4, 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!

Insti May 2, 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!

Insti left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

ajwann commented May 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Insti May 12, 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 May 14, 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!

Insti commented May 12, 2017 • 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

ajwann commented Apr 30, 2017 •

edited by Insti

Loading

ajwann Apr 30, 2017 •

edited

Loading

ajwann May 11, 2017 •

edited

Loading

ajwann May 4, 2017 •

edited

Loading

Insti May 2, 2017 •

edited

Loading

Insti commented May 2, 2017 •

edited

Loading

Insti May 12, 2017 •

edited

Loading

ajwann May 14, 2017 •

edited

Loading

Insti commented May 12, 2017 •

edited

Loading

Insti May 12, 2017 •

edited

Loading

Insti commented May 15, 2017 •

edited

Loading

Insti May 15, 2017 •

edited

Loading