Sg cleanup post cohost #11534
Sg cleanup post cohost #11534
Conversation
|
@dotnet/razor-compiler for review please |
There was a problem hiding this comment.
I love this change! As someone who spends far less time reading SG code than you have, I've really struggled to understand the data flow in this SG. Sp. I've left some feedback to call out spots that I feel would help readability more, a least for me. Feel free to take it or leave it.
Note that some of the feedback is duplicated in multiple places. That's mostly from just me doing a careful read through and trying not to miss something. 😉
| @@ -318,7 +309,7 @@ public void Initialize(IncrementalGeneratorInitializationContext context) | |||
|
|
|||
| context.RegisterImplementationSourceOutput(csharpDocumentsWithSuppressionFlag, static (context, pair) => | |||
| { | |||
| var ((hintName, _, csharpDocument), isGeneratorSuppressed) = pair; | |||
| var (hintName, _, csharpDocument, isGeneratorSuppressed) = pair; | |||
There was a problem hiding this comment.
Consider a name other than pair, since it now contains more than two pieces of data.
| @@ -344,7 +335,7 @@ public void Initialize(IncrementalGeneratorInitializationContext context) | |||
| context.RegisterHostOutput(hostOutputs, (context, pair) => | |||
| #pragma warning restore RSEXPERIMENTAL004 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed. | |||
| { | |||
| var ((documents, tagHelpers), isGeneratorSuppressed) = pair; | |||
| var (documents, tagHelpers, isGeneratorSuppressed) = pair; | |||
There was a problem hiding this comment.
Consider a name other than pair, since it now contains more than two pieces of data.
| @@ -48,7 +46,7 @@ public void Initialize(IncrementalGeneratorInitializationContext context) | |||
| .Combine(parseOptions) | |||
| .Combine(metadataRefs.Collect()) | |||
| .SuppressIfNeeded(isGeneratorSuppressed) | |||
| .Select(ComputeRazorSourceGeneratorOptions) | |||
| .Select((p, _) => ComputeRazorSourceGeneratorOptions(p.Item1.Item1, p.Item1.Item2, p.Item1.Item3, p.Item2)) | |||
There was a problem hiding this comment.
For clarity, consider adding named arguments for the values passed to ComputeRazorSourceGeneratorOptions. Or, add a body and deconstruct p into variables that are passed separately. Or, add a ComputeRazorSourceGeneratorOptions overload that take the expected ValueTuple with named members.
| { | ||
| var ((sourceItem, importFiles), razorSourceGeneratorOptions) = pair; | ||
| var (sourceItem, importFiles, razorSourceGeneratorOptions) = pair; |
There was a problem hiding this comment.
Consider a name other than pair, since it now contains more than two pieces of data.
| internal static IncrementalValuesProvider<ValueTuple<T1, T2, T3>> Combine<T1, T2, T3>(this IncrementalValuesProvider<ValueTuple<T1, T2>> provider, IncrementalValueProvider<T3> target) | ||
| { | ||
| return IncrementalValueProviderExtensions.Combine(provider, target) | ||
| .Select((p, _) => (p.Left.Item1, p.Left.Item2, p.Right)); |
There was a problem hiding this comment.
Would it be useful to have a Select extension that takes a lambda that is only passed one argument?
| .Combine(allTagHelpers) | ||
| .Select(static (pair, _) => | ||
| { | ||
| var (projectEngine, filePath, document, allTagHelpers) = pair; |
There was a problem hiding this comment.
Consider renaming document to codeDocument for consistency and to connect it to the data that has flowed from earlier.
|
|
||
| var csharpDocuments = processed(designTime: false) | ||
| var csharpDocuments = processed | ||
| .Select(static (pair, _) => | ||
| { | ||
| var (filePath, document) = pair; |
There was a problem hiding this comment.
Consider renaming document to codeDocument for consistency and to connect it to the data that has flowed from earlier.
| }) | ||
| .Select((pair, _) => | ||
| { | ||
| var (projectEngine, filePath, document) = pair; |
There was a problem hiding this comment.
Consider renaming document to codeDocument for consistency and to connect it to the data that has flowed from earlier.
|
|
||
| var document = projectEngine.ProcessInitialParse(sourceItem, designTime); | ||
| var document = projectEngine.ProcessInitialParse(sourceItem, designTime: false); |
There was a problem hiding this comment.
Consider renaming document to codeDocument since it will be deconstructed into codeDocument later.
| RazorSourceGeneratorEventSource.Log.GenerateDeclarationCodeStart(sourceItem.FilePath); | ||
|
|
||
| var projectEngine = GetDeclarationProjectEngine(sourceItem, importFiles, razorSourceGeneratorOptions); | ||
|
|
||
| var codeGen = projectEngine.Process(sourceItem); | ||
| var codeGen = projectEngine.Process(sourceItem, cancellationToken); |
There was a problem hiding this comment.
Love the cancellation! Should more of the RazorProjectEngine "Process" methods (like ProcessRemaining) take a CancellationToken as well?
Some minor cleanup refactoring to the source generator that I noticed while doing the cohosting work.