Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update test generator documentation. #560

Merged
merged 2 commits into from
Apr 21, 2017

Conversation

Insti
Copy link
Contributor

@Insti Insti commented Apr 21, 2017

Update the readme to better reflect the current best practices for test
generators.

Update the readme to better reflect the current best practices for test
generators.
@Insti Insti mentioned this pull request Apr 21, 2017
10 tasks
@hilary
Copy link
Contributor

hilary commented Apr 21, 2017

I would also suggest using a consistent name for the name method (say that 3 times fast!)

The sample lib/$PROBLEM_cases.rb uses test_name, as does the list below it, but the sample example.tt erb reads <%= test_case.name %>. Doh!

A quick scan of the existing generators shows that the consensus name is name. 😄

@Insti
Copy link
Contributor Author

Insti commented Apr 21, 2017

Hmm name might be the wrong thing to use due to potential conflicts with the json file.
I'll have to check.

name is definitely the right name.

Reasoning:

  • Desired usage in the erb (template) def <%= test_case.name %>
  • We should not need to care at this level what is in the canonical data json file
  • If the json causes a conflict this is a bug in ProblemCase creation code that needs to be fixed. (This bug does exist, but has not caused an issue ... yet.)

@kotp kotp merged commit 7ae5bec into exercism:master Apr 21, 2017
@Insti Insti deleted the Update_test_generator_documentation branch April 21, 2017 08:54
@Insti
Copy link
Contributor Author

Insti commented Apr 21, 2017

Addresses some of the issues discussed in: #485

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