Skip to content

Update README with Exercise Generators#478

Merged
Insti merged 2 commits intoexercism:masterfrom
bmulvihill:common-interface-readme
Nov 28, 2016
Merged

Update README with Exercise Generators#478
Insti merged 2 commits intoexercism:masterfrom
bmulvihill:common-interface-readme

Conversation

@bmulvihill
Copy link
Copy Markdown
Contributor

Related to: #452 and #463

Updates the README to include information about the generator interface. This way the README has up to date information before #463 is merged.

Comment thread README.md
@@ -108,6 +108,16 @@ then additional inputs/outputs should be submitted to the x-common repository.
Changes to the test suite (style, boilerplate, etc) will probably have to be made to
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.

Should we add "Track specific" before "Changes"?

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.

'track specific' might be redundant since this is only talking about the xruby track anyway.

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.

True enough. Was thinking this was the more general documentation.

Comment thread README.md Outdated
All generators currently adhere to a common public interface, and must define the following three methods:

- `test_name` - Output the name of the test
- `workload` - Output the body of the test
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.

Replace 'Output' with 'Return'
'Output' is ambiguous, since we don't want to be putsing things.

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.

I think there's still some discussion to be had about what workload should be returning.

I don't think 'the body of the test' is what it should be doing.

Copy link
Copy Markdown
Contributor Author

@bmulvihill bmulvihill Nov 1, 2016

Choose a reason for hiding this comment

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

Any thoughts about what it should do? Just return an assertion? If we are just returning an assertion than we might need to consider defining additional methods (i.e. input and expected, which can output variables that will be used in the workload method. I think it boils down to what belongs in the template vs the cases class.

Some recent generators (tournament for example) are defining an input and expect variable in the view, but the workload method is relying on the input variable being defined in the view, which does not seem ideal to me.

Keeping all the presentation confined to the workload method allows you to keep everything together at the same level. When I originally thought about the interface I was thinking the Cases classes would be almost like a presenter and workload would just spit out the test body. Since every exercise needs to define a test body, it is really just a matter of implementing its presentation.

Anyway these are just some of my thoughts, let me know what you think.

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.

@Insti @kotp Any thoughts on above?

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.

The template should be as simple as possible. It should contain as little "logic" as is feasible. This was the driving factor for skipped.

Copy link
Copy Markdown
Contributor

@Insti Insti Nov 5, 2016

Choose a reason for hiding this comment

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

I've been thinking about this.

Some recent generators (tournament for example) are defining an input and expect variable in the view, but the workload method is relying on the input variable being defined in the view, which does not seem ideal to me.

Keeping all the presentation confined to the workload method allows you to keep everything together at the same level.

And think I agree, but I would go even further and say that if workload is intended to generate the test then why not go all the way and have the generator generate the entire test case including the method definition and the skipped state. So the test generation part of the template should just become:

<% test_cases.each do |test_case| %>
  <%= test_case %>
<% end %>

This makes it much easier for the individual test cases to include comments (or even comment out entire tests.)

The [exercise]Case classes that currently inherit from Openstruct can
become children of a GenericTestCase class that implements a to_s method that outputs an entire test definition and implements the common methods like skipped? test_name, description_to_name and indent

So the documentation becomes, something about test cases needing a to_s method that outputs the entire test. And documenting the GenericTestCase class.

I'll see if I can find some time to make a working example.

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.

@Insti I would like to see it in action, it sounds interesting.

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.

@bmulvihill here is what I have so far: Insti#1

I'm working on this in my own fork until it is ready for merging into the main branch here. You should be able to pull the changes from my fork into your own if you want to test them out or submit PRs to me.

Everyone is welcome to comment/contribute.

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.

@Insti Awesome I'll pull it down and take a look

@bmulvihill bmulvihill force-pushed the common-interface-readme branch from 4da7ffa to 06fd378 Compare November 28, 2016 14:10
@bmulvihill
Copy link
Copy Markdown
Contributor Author

@Insti made some small updates based on your review. I think this should work until your changes are made to the generator.

@Insti Insti merged commit 3ffe6bf into exercism:master Nov 28, 2016
@Insti
Copy link
Copy Markdown
Contributor

Insti commented Nov 28, 2016

Great, thanks. It's good to document the current process even if there is a new one coming (slowly).

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.

3 participants