Generators refactoring by ErikSchierboom · Pull Request #601 · exercism/csharp

ErikSchierboom · 2018-06-25T07:27:49Z

This is to test the newly enabled WIP plugin (see exercism/discussions#217 (comment)). And it will also give insight into how my refactoring attempt is progressing.

…ing>

…Method

… to customize output

ErikSchierboom · 2018-07-08T18:34:54Z

Finally, after much work, I'm done with my huge generators refactoring. It has become even bigger that the F# equivalent, and it contains the following improvements:

Simplified extension points available to generators to three virtual methods.
Removed as many of the classes the user should know about.
Greatly simplified rendering code.
Make data rendering code generic (handle any dictionary/array/list instead of only a couple of hardcoded ones).
Many places in which the generator code has been simplified.
Fixed a bug where the test methods were not always in the correct order when using nested canonical data cases.
Made canonical data and canonical data case classes immutable.
Allow testing of properties and constructors.
Use ValueTuple<T> where possible.
Add MultilineString class to allow generators to be explicit on how they want the data to be output.
Add helper method to render enumerations.
Remove unused code.
Updated documentation with more examples of common use cases.
Restructured namespaces to make the code have a better structure.
And just overall many, many improvements to make the code more readable.

CC @robkeim @jpreese

ErikSchierboom · 2018-07-09T08:22:02Z

@robkeim @jpreese If this PR is too big too review, no sweat. If you do want to review, it might be best to just check out this branch and examine the code locally.

robkeim · 2018-07-09T15:05:34Z

@ErikSchierboom I'm feeling brave... I'll take a look :)

ErikSchierboom · 2018-07-09T15:12:00Z

@robkeim Great! I'm really interested in what you think.

robkeim

Awesome work @ErikSchierboom! Super cool stuff 👍

Added a few comments and reviewed through CollatatzConjecture so far.

robkeim · 2018-07-09T15:11:05Z

-Additional methods added using this override will be added to the bottom of the test suite.
+In some case, you might want to override the parameters that are used as input parameters.
+
+An example of this is the [two fer]() generator, which does not use any input parameters when the `"name"` input parameter is set to `null`:


Missing url here

robkeim · 2018-07-09T15:11:20Z

+
+If a test method tests an instance method, you can also specify which parameters to use as constructor parameters (the others will be input parameters, unless specified otherwise).
+
+An example of this is the [matrix]() generator, which specifies that the `"string"` parameter should be passed as a constructor parameter:


Here as well

robkeim · 2018-07-09T15:11:35Z

+Some test methods want to verify that an exception is being thrown.

-Here the **Assert** is being overridden. The assert needs to call additional functions, but only if the property is `consistency`. Otherwise, render the assert as usual.
+An example of this is the [RNA transcription]() generator, which defines that some test methods should throws an `ArgumentException`:


Missing url here

robkeim · 2018-07-09T15:13:00Z

+
+This method allows you to customize the output of the test class. Only in rare cases would you want to override this method. The most common use case to override this method, is to add additional (helper) methods to the test suite.
+
+An example of this is the [Tournament](https://github.com/exercism/csharp/blob/master/generators/Exercises/Tournament.cs) generator, which adds a helper method to the test suite:


Minor: tournament should be lowercase to be consistent with the other test names

robkeim · 2018-07-09T15:13:35Z

    {
        var inputBase = 2;
-        var digits = new int[0];
+        var digits = Array.Empty<int>();


robkeim · 2018-07-09T15:15:11Z

    {
        var sut = new BowlingGame();
-        var previousRolls = new [] { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 7, 3, 7 };
+        var previousRolls = new[]


Why are these all on separate lines but the previous tests aren't? IMO it's probably better to have it on one line

I guess there are a few other occurrences in the rest of this test file.

Well, this is me trying to be clever. Many of the input arrays have a very limited number of elements (e.g. 1 or 2), in which case rendering them over multiple lines would be overkill IMHO. The code that does this check can be found here. It essentially checks if it is not an array of arrays and if the rendered single line output would be less than or equal to 72 characters. If so, it is output as a single line.

This is of course completely up for debate. I'd be happy to hear your thoughts on this.

What I could do, is detect how many lines would end up being created. If there are more than a given number, say 8, then I could put multiple elements on a single line. Would that be an option?

@robkeim I've given it some more thought, and I think I was attempting to be a bit too clever. I've removed the automatic multi-line rendering of single-dimensional arrays. Exercises can choose to opt-in themselves, should they want to do multi-line array initialization.

robkeim · 2018-07-09T15:20:31Z

-            new ValueTuple<int, int>(col+1, row),
-            new ValueTuple<int, int>(col,   row+1)
+            (col,   row - 1),
+            (col-1, row),


Super minor: missing spaces around the - here and the + below

robkeim · 2018-07-09T15:24:23Z

-        var expected = new string[] 
-        { 
-            "" 
+        var minefield = new[]


Minor: the fact that each row is a separate line here vs in the expected it's on one line makes this definitely look like it wasn't handwritten.

robkeim · 2018-07-09T15:27:51Z

    public void Find_primes_up_to_1000()
    {
-        var expected = new[] { 2, 3, 5, 7, 11, 13, 17, 19, 23, 29, 31, 37, 41, 43, 47, 53, 59, 61, 67, 71, 73, 79, 83, 89, 97, 101, 103, 107, 109, 113, 127, 131, 137, 139, 149, 151, 157, 163, 167, 173, 179, 181, 191, 193, 197, 199, 211, 223, 227, 229, 233, 239, 241, 251, 257, 263, 269, 271, 277, 281, 283, 293, 307, 311, 313, 317, 331, 337, 347, 349, 353, 359, 367, 373, 379, 383, 389, 397, 401, 409, 419, 421, 431, 433, 439, 443, 449, 457, 461, 463, 467, 479, 487, 491, 499, 503, 509, 521, 523, 541, 547, 557, 563, 569, 571, 577, 587, 593, 599, 601, 607, 613, 617, 619, 631, 641, 643, 647, 653, 659, 661, 673, 677, 683, 691, 701, 709, 719, 727, 733, 739, 743, 751, 757, 761, 769, 773, 787, 797, 809, 811, 821, 823, 827, 829, 839, 853, 857, 859, 863, 877, 881, 883, 887, 907, 911, 919, 929, 937, 941, 947, 953, 967, 971, 977, 983, 991, 997 };
+        var expected = new[]


This could probably be on one line or a few lines instead of one line per number

See comment above.

jpreese · 2018-07-09T19:30:11Z

haha, oh man.. where to start. Yeah, let me check this out locally and try to make sense of it all. Should hopefully have some time later in the week.

…r minesweeper

ErikSchierboom · 2018-07-10T10:19:47Z

Although I do think the commits are really useful to see the gradual evolution of the refactoring, I think that squashing this will be the way to go when merging this PR.

… arrays

jpreese · 2018-07-14T19:36:11Z

+#### Customize test data

-As an example, if you wanted to change the default behavior so that when the `Input` value of a test is a negative number, an exception should be thrown, the code would look like this.
+There are quite some generators that want to change the input or expected value.


This sentence doesn't seem quite right

jpreese · 2018-07-14T19:37:17Z

-As an example, if you wanted to change the default behavior so that when the `Input` value of a test is a negative number, an exception should be thrown, the code would look like this.
+There are quite some generators that want to change the input or expected value.
+
+An example of this is the [bracket-push](https://github.com/exercism/csharp/blob/master/generators/Exercises/Generators/BracketPush.cs) generator, which input value with the key `"value"` requires some additional escaping:


This sentence doesn't seem quite right

jpreese · 2018-07-14T19:42:04Z

+Some test methods want to verify that an exception is being thrown.

-Here the **Assert** is being overridden. The assert needs to call additional functions, but only if the property is `consistency`. Otherwise, render the assert as usual.
+An example of this is the [rna-transcription](https://github.com/exercism/csharp/blob/master/generators/Exercises/Generators/RnaTranscription.cs) generator, which defines that some test methods should throws an `ArgumentException`:


should throw*

jpreese · 2018-07-14T19:42:39Z

+#### Custom input/constructor parameters

-Additional methods added using this override will be added to the bottom of the test suite.
+In some case, you might want to override the parameters that are used as input parameters.


jpreese · 2018-07-14T19:44:09Z

+}
+```
+
+Note that as mentioned before, the namespace of any thrown exception types are automaticall added to the list of namespaces.


automatically*

jpreese · 2018-07-14T20:07:03Z

That review destroyed by browser.

ErikSchierboom · 2018-07-15T07:59:03Z

@jpreese Thanks for the review. I've updated the docs.

robkeim

Awesome job @ErikSchierboom!!! Super high quality code as always 👍

ErikSchierboom · 2018-07-15T16:38:56Z

Thanks a ton for reviewing @robkeim and @jpreese!

ErikSchierboom added 29 commits June 17, 2018 16:27

generators: Ignore Rider settings files

1c0dbc2

generators: Refactor additional namespaces

d510410

exercises: Update to latest canonical data

2ae70ff

exercises: Use Array.Empty<T> instead of new T[0]

5123f65

generators: Use static class

5a43560

generators: Remove unused using statements

dc72c27

generators: Move exercise field to canonical data case class

3fa390b

generators: Use environment specific newline

745cc84

generators: Fix culture-specific output

0d08690

generators: Replace UpdateCanonicalData with UpdateCanonicalDataCase

48779f2

generators: Test method body rendering methods return IEnumerable<str…

2a62574

…ing>

generators: Use var everywhere

153376a

generator: Code cleanup

6f4cc49

generators: Remove unused parameter

ff1f7bc

generators: Move variables data to TestMethodData

096dcab

generators: Make canonical data class immutable

67fefe6

generators: Extract rendering logic from test data to test method body

55e4ff7

generators: Replace Exercise and Property with TestedClass and Tested…

bbc999b

…Method

generators: Update package versions

998873c

generators: Don't use sealed

ea4586b

generators: Use UpdateTestClass/UpdateTestMethod/UpdateTestMethodBody…

245e122

… to customize output

exercises: Sort namespaces

9a81135

generator: Make GeneratorExercise API more consistent

6de8d02

generators: Remove unneeded initialization

0c9ddd8

generators: Rename ExerciseWriter to TestClassFile

4ce46a9

generators: Move template specific code to separate namespace

51caf22

generators: Move helpers to separate namespace

9fe5b48

generators: Remove unneeded generator scripts

761ee44

generators: Update namespace

5c246fb

ErikSchierboom mentioned this pull request Jun 25, 2018

Enable WIP plugin to prevent WIP merges exercism/discussions#217

Closed

ErikSchierboom added 3 commits July 8, 2018 19:18

generators: Simplify flatten-array generator

f2d030d

generators: Refactor test method output

816b0a9

generators: Update docs

8eb23b9

ErikSchierboom changed the title ~~WIP Generators refactoring~~ Generators refactoring Jul 8, 2018

ErikSchierboom requested a review from jpreese July 8, 2018 18:35

This was referenced Jul 9, 2018

Grade school generator #602

Merged

Diffie hellman generator #603

Merged

Series generator #604

Merged

robkeim reviewed Jul 9, 2018

View reviewed changes

ErikSchierboom added 2 commits July 9, 2018 21:24

generators: Add missing links to docs and use consistent naming

cc593f9

go-counting: Add space between operators in example implementation

fbe4a34

generators: Use consistent formatting of input and expected values fo…

523748a

…r minesweeper

ErikSchierboom added 2 commits July 10, 2018 19:04

generators: Increase length for single-line array initializers

e50998c

generators: Remove automatic multi-line rendering for one-dimensional…

2764341

… arrays

jpreese suggested changes Jul 14, 2018

View reviewed changes

generators: Update docs

e46f687

robkeim approved these changes Jul 15, 2018

View reviewed changes

jpreese approved these changes Jul 15, 2018

View reviewed changes

ErikSchierboom merged commit e3142cb into exercism:master Jul 15, 2018

ErikSchierboom deleted the generators-refactoring branch July 15, 2018 16:38

ErikSchierboom mentioned this pull request Mar 31, 2021

🤖 Add authors and contributors to Practice Exercises #1519

Merged

4 tasks


		If a test method tests an instance method, you can also specify which parameters to use as constructor parameters (the others will be input parameters, unless specified otherwise).

		An example of this is the [matrix]() generator, which specifies that the `"string"` parameter should be passed as a constructor parameter:


		This method allows you to customize the output of the test class. Only in rare cases would you want to override this method. The most common use case to override this method, is to add additional (helper) methods to the test suite.

		An example of this is the [Tournament](https://github.com/exercism/csharp/blob/master/generators/Exercises/Tournament.cs) generator, which adds a helper method to the test suite:

Uh oh!

Conversation

ErikSchierboom commented Jun 25, 2018

Uh oh!

ErikSchierboom commented Jul 8, 2018

Uh oh!

ErikSchierboom commented Jul 9, 2018

Uh oh!

robkeim commented Jul 9, 2018

Uh oh!

ErikSchierboom commented Jul 9, 2018

Uh oh!

robkeim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpreese commented Jul 9, 2018

Uh oh!

ErikSchierboom commented Jul 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpreese commented Jul 14, 2018

Uh oh!

ErikSchierboom commented Jul 15, 2018

Uh oh!

robkeim left a comment

Choose a reason for hiding this comment

Uh oh!

ErikSchierboom commented Jul 15, 2018

Uh oh!

Reviewers

Assignees