Rich generators #80
Rich generators #80
Conversation
|
@AArnott ping? What are your thoughts? #Closed |
|
@AArnott pinging again. Any reaction? #Closed |
|
|
||
| <PropertyGroup> | ||
| <TargetFramework>netstandard1.6</TargetFramework> | ||
| <LangVersion>7.1</LangVersion> |
There was a problem hiding this comment.
Let's go ahead and shoot for 7.3. 😄 #Closed
* Bump C# lang version in main project to 7.3 * Rename variables in DocumentTransform to start with 'emitted' as original 'emittedMembers' * Move old behavior of wrapping ICodeGeneration results with ancestor types into EnrichingCodeGeneratorProxy * Unify calling code generators into calling IRichCodeGenerators, wrapping normal ones in the proxy class
|
@AArnott I've added tests, done some refactors and bumped lang ver to 7.3 as requested. I think I'm done here. What's your take on this? #Closed |
|
One major refactor I should maybe explain is the move of wrapping-member-in-ancestor logic; I've moved it into a separate nested class which I've named So the processing is now more streamlined: non-rich generators are now wrapped into Proxy class, and only the Rich generation is called in the One other thing I've been uncertain, is: should the |
|
@AArnott pinging for review :) #Closed |
|
Sorry... looking now.. #Resolved |
|
|
||
| private class EnrichingCodeGeneratorProxy : IRichCodeGenerator | ||
| { | ||
| public EnrichingCodeGeneratorProxy(ICodeGenerator codeGenerator) |
There was a problem hiding this comment.
codeGenerator [](start = 62, length = 13)
Since we dereference this later without checking, please add Requires.NotNull(codeGenerator, nameof(codeGenerator)); to this ctor. #Closed
|
I don't see how a factory method will make it more future-proof than a ctor. We would presumably add more of either one as we add members. However, the fact that you're already using default parameters on the first "overload" may make it tricky to add more overloads with default parameters later. I'll leave more comments in the relevant files. In reply to: 413479597 [](ancestors = 413479597) |
| SyntaxList<UsingDirectiveSyntax> usings = default, | ||
| SyntaxList<AttributeListSyntax> attributeLists = default, | ||
| SyntaxList<ExternAliasDirectiveSyntax> externs = default) | ||
| { |
There was a problem hiding this comment.
Instead of these ctors, which will be troublesome to version as we add more members, why not give the properties public setters and let them set whatever ones they want? #Closed
| attributeLists = AttributeLists; | ||
| externs = Externs; | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not a fan of deconstruction unless it's very obvious what the members will be to the caller and what order they should come in (i.e. first and last name). As the members grow, their order in this list are basically arbitrary -- they look arbitrary right now. That leads to bugs in code when you add/refactor in this class but fail to update every caller (which is hard, since they don't actually call Deconstruct explicitly) and it happens to compile because the types between two reordered members remains the same.
It also requires that the caller explicitly name the variables when it would produce smaller, more readable code if they just dereferenced the property as it is defined in this struct each time. #Closed
| string projectDirectory, | ||
| IEnumerable<UsingDirectiveSyntax> compilationUnitUsings, | ||
| IEnumerable<ExternAliasDirectiveSyntax> compilationUnitExterns) | ||
| { |
There was a problem hiding this comment.
While this is a technically breaking change, it should only break unit tests that may have been written for generators, which isn't as severe.
We could make this internal, but that would make mocking up those unit tests even harder. So I'm good with this change. #ByDesign
|
I'm glad I could help! All the comments were valid and I see the point. I've got a little carried away on that immutability bandwagon. :) |
It's just an initial concept draft to start up a discussion
I've got an idea for a big enhancement: a feature that would enable many more scenarios for code generation in this framework, namely:
[assembly: Xyz])My proposal adds a new interface that has a method with the same parameter list as current
ICodeGenerator.GenerateAsync, but returnsTask<RichGenerationResult>which contains four syntax lists: Members, Usings, Externs and AttributeLists that get merged with those created by previous generators and then are added to the generated compilation unit.There are no tests currently, but I will add them, if the design would be accepted.