Skip to content

bin/generator: Fix outstanding TODOs#540

Merged
kotp merged 7 commits intoexercism:masterfrom
Insti:Fix_generator_todos
Jan 17, 2017
Merged

bin/generator: Fix outstanding TODOs#540
kotp merged 7 commits intoexercism:masterfrom
Insti:Fix_generator_todos

Conversation

@Insti
Copy link
Copy Markdown
Contributor

@Insti Insti commented Jan 14, 2017

This patch addresses the outstanding TODOs in the recent generator refactor.

Changes all exercise.tt files:

  • Remove compensation for version increment in templates.
  • Rename sha1 to abbreviated_commit_hash
  • Standardise common test data reference comment.

Changes all lib/*_cases.rb files:

  • move the exercise cases requires out of the generator.

I was initially planning on doing this as 3 Pull Requests, but the default git settings weren't quite clever enough to work out how to merge the independent changes without conflicts so I've applied them all in one PR. The individual commits show the independent steps.

Comment thread test/generator/template_values_test.rb Outdated

def test_get_binding
subject = TemplateValues.new(sha1: nil,version: nil, test_cases: nil)
subject = TemplateValues.new(abbreviated_commit_hash: nil,version: nil, test_cases: nil)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's causing the lack of space in nil,version?

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.

xruby using a rubocop.yml with all the useful cops disabled.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hehe, I think that is allowing it, rather than causing it. I like adding in that one. If you would like to run with a "custom" rubocop configuration, you can...

rubocop -c ~/.sane_rubocop.yml

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.

Well then I guess what caused it was me not hitting the spacebar between , and v.

If it's important (which it seems to be, if you're noticing it in a code review.) it should be checked for by the xruby Rubocop configuration and not rely on a different unmaintained configuration file.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am noticing it by sight, rather than by tool, they stand out to me.

The point of me mentioning the custom configuration is not as a solution to this, but in the use of that for individual contributors, for spotting the things that they care about... mentioning it here and hopefully having those things that are deemed important enough to come into the configuration here.

I like how this rubocop change happened here. Naturally happening, and as needed.

Insti added 7 commits January 17, 2017 10:32
The responsibility for maintaining the template version belongs with the
generator, not the template.
Once the templates have been updated, we no longer need to compensate
for their version incrementing.
The common requirements now live in the `lib/exercise_cases.rb` file and
this is included by the individual cases files rather than being loaded
by the generator code.
@Insti Insti force-pushed the Fix_generator_todos branch from ee57e85 to 7c7e32b Compare January 17, 2017 10:35
@Insti
Copy link
Copy Markdown
Contributor Author

Insti commented Jan 17, 2017

Rebased to resolve merge conflict.
This should be ready to merge now.
These commits should NOT be squashed.

@kotp kotp merged commit f4f9907 into exercism:master Jan 17, 2017
@Insti Insti deleted the Fix_generator_todos branch January 17, 2017 12:42
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.

2 participants