[WIP] generator: refactor implementation, part 1 by hilary · Pull Request #641 · exercism/ruby

hilary · 2017-05-14T23:24:09Z

I did this PR in two parts. The first part (this one) separates out the Exercise model.

The second part (two different PRs) separates out the Repository model in two different ways. One PR creates a Repository which is instantiated with paths only. The second PR creates a Repository which is instantiated with paths and exercise.

#available shouldn't depend on the existence of a '<mumble>_case.rb' file, because that might go away or get changed. We can just look for the presence of a .meta/generator directory.

leftover from the proc days

Two reasons. Main reason: Implementation shouldn't be thinking in terms of files. Secondary reason, old name was misleading, as it regenerates the file when it exists :(

Insti · 2017-05-19T07:17:30Z

-    def test_slug
-      subject = Implementation.new(paths: FixturePaths, slug: 'alpha')
-      assert_equal 'alpha', subject.slug
-    end


This is not a test of core ruby functionality, it is a test that Implementation responds to slug.

Request: Keep this test.

Edit: But it needs to be deleted in the next commit anyway (for a different reason), so squash these two commits together.

Insti · 2017-05-19T07:21:24Z

+
+      def camel_case
+        underscore.split('_').map(&:capitalize).join
+      end


I'm sceptical of any change that is an "add" as part of a 'refactor' commit.

This change makes the class name Underscore inaccurate.

Insti · 2017-05-19T07:27:55Z

    module GeneratorCases
      class << self
        def available(track_path)
-          cases_filepaths(track_path).map { |filepath| slugify(filepath) }.sort


I strongly disagree with this change.

It is true that available does not need to know exactly how a test case is generated, and you could achieve this by abstracting out the availability criteria, but since that is all this class does that seems excessive.

because that might go away or get changed.

This is a case of YAGNI and we can worry about that when it happens.

See also: #646 which aims to give new generator implementers helpful advice.

Request: Omit this commit.

Insti · 2017-05-19T07:31:55Z

+
+    def test_name
+      exercise = Exercise.new(slug: 'alpha-beta')
+      assert_equal 'alpha_beta', exercise.name


Yay for tests.
I'm not sure name is the right name for this property, but it is consistent with what we've been using.

Insti · 2017-05-19T07:35:11Z

    end

-    class ClassBasedTestTemplateValuesFactory
+    class ClassBasedTestTemplateValuesFactory < TestTemplateValuesFactory


You like tests to be way more DRY than I do.
I'm not sure this is a good idea.
(But it's not wrong.)

Insti · 2017-05-19T07:37:28Z

      include TemplateValuesFactory
    end

-    class ClassBasedTestTemplateValuesFactory < TestTemplateValuesFactory


It goes away anyway. Yay.

Insti · 2017-05-19T07:38:30Z

      }
    end

-    def test_abbreviated_commit_hash


Again, not core ruby functionality.
That it is implemented using an attr_reader is irrelevant.

Request: omit this commit.

Insti · 2017-05-19T07:43:05Z

+
    def exercise_name_camel
-      exercise_name.split('_').map(&:capitalize).join
+      exercise.name.camel_case


slug is the canonical value for problem identity.
This should be exercise.slug.camel_case
But either way it's a Demeter violation, so should be just exercise.test_class_name

exercise_name_camel is a slightly misleading method name here given it's usage is to determine the test class name used in the template. But this should be a separate PR/refactoring. Issue: #651

Insti · 2017-05-19T07:58:21Z

@@ -3,20 +3,27 @@ module Generator
  class TemplateValues


The original intention of TemplateValues was that it it should be a dumb container and would only contain the template values available to the erb file. The initialize method provided one place where it was possible to see exactly all the properties that would be available to the erb.

It is the responsibility of the TemplateValuesFactory to construct the necessary values.

If that is the goal, then there is probably a nicer way of implementing this but I'm not sure this commit is heading in that direction.

Insti · 2017-05-19T07:58:56Z

  # Doesn't update the version information.
  class GenerateTests < ImplementationDelegator
    def call
-      create_tests_file


I like this 👍

Insti

Nice work.

Many of my comments apply to the whole commit the comment is part of rather than that specific place.

I've marked things I feel strongly need changing with Request:
(beware: Github has hidden some of them under show outdated)

Insti · 2017-06-13T19:00:06Z

I've extracted the best parts from this PR in: #677 and will continue the refactoring from there.

hilary added 10 commits May 14, 2017 11:57

use correct version of test_template.erb

2778649

add camel_case method to underscore

ac3b558

refactor GeneratorCases#available

39e5dae

#available shouldn't depend on the existence of a '<mumble>_case.rb' file, because that might go away or get changed. We can just look for the presence of a .meta/generator directory.

don't test core ruby functionality

3d33f3c

introduce Exercise model

96c47ea

remove redundant test

8f870a6

leftover from the proc days

don't test core ruby functionality

e2b41c2

pass exercise to TemplateValues

892acf2

pass canonical_data to TemplateValues

d0e4205

rename create_tests_file to build_tests

9dfc1f8

Two reasons. Main reason: Implementation shouldn't be thinking in terms of files. Secondary reason, old name was misleading, as it regenerates the file when it exists :(

This was referenced May 14, 2017

[WIP] generator: part 2 of refactor implementation model #642

Closed

[WIP] generator: part 2 (version b) of refactor implementation model #643

Closed

hilary requested a review from Insti May 14, 2017 23:28

Insti changed the title ~~generator: refactor implementation, part 1~~ [WIP] generator: refactor implementation, part 1 May 18, 2017

Insti added the in progress label May 18, 2017

Insti reviewed May 19, 2017

View reviewed changes

Insti suggested changes May 19, 2017

View reviewed changes

Insti mentioned this pull request Jun 13, 2017

generator: refactor implementation, part 1 highlights. #677

Merged

Insti closed this Jun 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[WIP] generator: refactor implementation, part 1#641

[WIP] generator: refactor implementation, part 1#641
hilary wants to merge 10 commits intoexercism:masterfrom
hilary:refactor_implementation

hilary commented May 14, 2017

Uh oh!

Insti May 19, 2017 •

edited

Loading

Uh oh!

Insti May 19, 2017

Uh oh!

Insti May 19, 2017 •

edited

Loading

Uh oh!

Insti May 19, 2017

Uh oh!

Insti May 19, 2017

Uh oh!

Insti May 19, 2017

Uh oh!

Insti May 19, 2017

Uh oh!

Insti May 19, 2017 •

edited

Loading

Uh oh!

Insti May 19, 2017

Uh oh!

Insti May 19, 2017

Uh oh!

Insti left a comment •

edited

Loading

Uh oh!

Insti commented Jun 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

hilary commented May 14, 2017

Uh oh!

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

Insti May 19, 2017

Choose a reason for hiding this comment

Uh oh!

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

Insti May 19, 2017

Choose a reason for hiding this comment

Uh oh!

Insti May 19, 2017

Choose a reason for hiding this comment

Uh oh!

Insti May 19, 2017

Choose a reason for hiding this comment

Uh oh!

Insti May 19, 2017

Choose a reason for hiding this comment

Uh oh!

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

Insti May 19, 2017

Choose a reason for hiding this comment

Uh oh!

Insti May 19, 2017

Choose a reason for hiding this comment

Uh oh!

Insti left a comment • 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 commented Jun 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Insti May 19, 2017 •

edited

Loading

Insti May 19, 2017 •

edited

Loading

Insti May 19, 2017 •

edited

Loading

Insti left a comment •

edited

Loading