Source generator tweaks #262
Conversation
Primarily moves processing only to the branches that need it. Additionally makes the comment field not-nullable as we perform an initial check at the beginning of the Read method on SchemaPropertyJsonConverter.
Technically the code was asynchronous before however the initial request would buffer internally, which we then serialize to a string and parse back out. Now we start processing the stream as we receive the content - improving performance and decreasing allocations.
Turnerj
added
the
enhancement
Confirmed - initialization and executing occur multiple times. Next check will be if they use the same instance of the generator and if they do, we can skip the initialization if the Example Log
|
Unfortunately they use different instances however as a static property, we can share data between them. The multi-target builds seem to overlap so adding some locking to it seems like a good idea. |
| @@ -9,8 +9,8 @@ public class SchemaClass : SchemaObject | |||
| { | |||
| private static readonly Uri EnumerationId = new("https://schema.org/Enumeration"); | |||
|
|
|||
| public SchemaClass(Uri id, string label, string layer) | |||
| : base(id, label, layer) | |||
| public SchemaClass(string comment, Uri id, string label, string layer) | |||
There was a problem hiding this comment.
Minor comment. It would be nice if the order of parameters in these constructos followed the hierarchy of data.
string layer, Uri id, string label, string comment
There was a problem hiding this comment.
Happy to do that. While I know what hierarchy means, I'm not sure exactly how you're deriving it - like these properties exist in the same hierarchy of inheritance. Just want to understand how you want it.
I went alphabetical for this but felt weird to have comment first.
There was a problem hiding this comment.
I think layer comes first because it groups many types together. The ID for a type makes sense next as it is the unique identifier. After that the label seems to make sense to me, although I can't explain why. After these three, I think going with alphabetical ordering is fine, unless it makes sense to group parameters together somehow.
|
Nice nice improvements. Do you think we can release a major version after this? |
|
We can, yeah. I do want to try and get pending types but that may be better after. |
Changes the order of constructor parameters and also pushes description/comment and "IsCombined" into constructors, making the properties immutable.
|
LGTM |
RehanSaeed
added
the
patch
Various source generator tweaks and improvements:
schema:domainIncludesfor properties only)HttpCompletionOptionwhich helps this) which can improve performance and allocationsTask.Runin the generator's initializerOther plans:
Investigate whether the source generator is triggered for every target (if so, see if we can cache the JSON)Other notes:
dotnet buildbut notmsbuildor via Visual Studio. See Source generator dependency unable to be resolved dotnet/roslyn#52017 - the CI isn't affected as far as I can tell.