Simplify generators#302
Simplify generators#302robkeim merged 22 commits intoexercism:masterfrom ErikSchierboom:refactoring-generators
Conversation
| { | ||
| public class Acronym : EqualityExercise | ||
| { | ||
| public Acronym() : base("acronym") |
There was a problem hiding this comment.
Another potential simplification would be to generate the exercise name from the Class name using reflection. The rules are pretty simple:
- Acronym -> acronym
- NthPrime -> nth-prime
You could argue it actually makes the code more obscure but it's some food for thought :)
There was a problem hiding this comment.
I like it! The PR has been updated. Don't merge it yet, as there is still a problem with the multi-line formatting in beer-song. I think that's the next thing I want to get right: figuring out how to best configure how the output is formatted.
| Index = index, | ||
| Options = CreateTestMethodOptions(canonicalData, canonicalDataCase, index) | ||
| }; | ||
| if (testMethodData.Options.TestedMethodType == TestedMethodType.BooleanComparison) |
There was a problem hiding this comment.
Minor: To future proof this you may want to have a switch with the two types existing today and then throw an exception for the default. That way if we need to add a new type in the future it's clear where it needs to be plumbed through to.
There was a problem hiding this comment.
I like this!
edit: I have to modify it slightly, as the expection must be tested before the switch
| case TestedMethodFormat.Static: | ||
| return new[] { $"{Assertion}({TestedClassName}.{TestedMethodName}({Input}));" }; | ||
| case TestedMethodFormat.Instance: | ||
| return new[] { $"var sut = new {TestedClassName}();", $"{Assertion}(sut.{TestedMethodName}({Input}));" }; |
There was a problem hiding this comment.
It stands for "system under test" and is common jargon in some test frameworks.
There was a problem hiding this comment.
Nice, you learn something new everyday!
| if (lines.Length == 1) | ||
| return lines[0]; | ||
|
|
||
| return "\n" + string.Join("\n", lines.Select(line => $"{line}")); |
There was a problem hiding this comment.
What's the difference between what you have an line => line?
There was a problem hiding this comment.
Ah, it used to have tabs! I'll fix it.
| { | ||
| public enum TestedMethodFormat | ||
| { | ||
| Static = 0, |
There was a problem hiding this comment.
Minor: no need to start with 0 as it's the default
robkeim
left a comment
robkeim
left a comment
There was a problem hiding this comment.
Chatted offline about this and it looks great @ErikSchierboom! Once you've got the beer-song bug fixed either you can merge or if you'd like me to look at the finishing touches I'd be happy to do that and merge as well.
|
@robkeim I've made some further simplifications that I think turned out quite nice :) There are two major open points left:
|
robkeim
left a comment
robkeim
left a comment
There was a problem hiding this comment.
Looks good @ErikSchierboom!
|
@robkeim I've updated the PR and I am happy with the end result. Let me know how you feel! |
|
Excellent work @ErikSchierboom, thanks for the hard work! |
2 participants
This is the first step in refactoring the test generators. The aim is to make it really, really simple. In this PR, we default to using all the non-default properties as input properties. In a future PR I would like to get rid of the virtual methods as much as possible. Why not use constructor parameters instead, or otherwise use properties that can be set in the constructor?
I've also used used @robkeim's suggestion of losing the "Exercise" postfix for the exercises.