V3: Option to have lazy DSC<> initialization. Option to exclude static model #46
Conversation
|
Hi @uffelauesen, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! The agreement was validated by Microsoft and real humans are currently evaluating your PR. TTYL, MSBOT; |
| @"private abstract class RuntimeEdmModel | ||
| { | ||
| [global::System.CodeDom.Compiler.GeneratedCodeAttribute(""System.Data.Services.Design"", ""1.0.0"")] | ||
| private static global::System.Collections.Generic.Dictionary<string, global::Microsoft.Data.Edm.IEdmModel> models = new global::System.Collections.Generic.Dictionary<string, global::Microsoft.Data.Edm.IEdmModel>(global::System.StringComparer.OrdinalIgnoreCase); |
There was a problem hiding this comment.
Please split into multiple lines for readability.
| if (generator.UseDataServiceCollection) | ||
| { | ||
| await this.Context.Logger.WriteMessageAsync(LoggerMessageCategory.Information, "Proxy - injecting lazy initialization of DataServiceCollections"); | ||
| pattern = @"private (.+) _(.+) = new global::System.Data.Services.Client.DataServiceCollection\<(.+)\>\(null, global::System.Data.Services.Client.TrackingMode.None\);"; |
There was a problem hiding this comment.
Please split into multiple lines for readability.
| int loadServiceModelAssignmentIndex = modified.IndexOf(loadServiceModelAssignment); | ||
| if (loadServiceModelAssignmentIndex >= 0) | ||
| { | ||
| modified = string.Concat(modified.Substring(0, loadServiceModelAssignmentIndex), "this.Format.LoadServiceModel = () => RuntimeEdmModel.LoadModel(this);", modified.Substring(loadServiceModelAssignmentIndex + loadServiceModelAssignment.Length)); |
There was a problem hiding this comment.
Please split into multiple lines for readability.
| modified = Regex.Replace(original, pattern, collectionReplacer); | ||
| } | ||
| else | ||
| modified = original; |
There was a problem hiding this comment.
Nit: please encapsulate this in { }, even for one-liners to be consistent with the style standard.
| @@ -16,6 +16,67 @@ namespace Microsoft.OData.ConnectedService.CodeGeneration | |||
| { | |||
| internal class V3CodeGenDescriptor : BaseCodeGenDescriptor | |||
| { | |||
| const string loadServiceModelAssignment = "this.Format.LoadServiceModel = GeneratedEdmModel.GetInstance;"; | |||
| const string dynamicModelLoader = | |||
There was a problem hiding this comment.
For maintainability, can you please put a comment with a high-level description on how the class works/will be utilized?
| private static global::System.Collections.Generic.Dictionary<string, global::Microsoft.Data.Edm.IEdmModel> models = new global::System.Collections.Generic.Dictionary<string, global::Microsoft.Data.Edm.IEdmModel>(global::System.StringComparer.OrdinalIgnoreCase); | ||
|
|
||
| [global::System.CodeDom.Compiler.GeneratedCodeAttribute(""System.Data.Services.Design"", ""1.0.0"")] | ||
| private static object modelsCacheLock = new object(); |
There was a problem hiding this comment.
Please make this lock readonly as well.
…ingField to avoid WCF DataServicesClient triggering collection/DSC<> construction when doing null check.
|
Some "build trigger has fired" - I can't access the Details, if I need to change something please let me know. |
|
@uffelauesen , the build trigger that fired failed but it's a separate problem and not due to the changes you've made. So no worries there. :) Thanks for being so quick addressing the comments that I wrote. I've been putting thought into the strategy that you're using, specifically that you're modifying the output provided by Microsoft.Data.Services.Design.dll in V3CodeGenDescriptor.cs:
In general, we should not modify the generated text output and instead, update the algorithm that generates the text. The reason is that manipulating the output in this manner increases complexity for testing and maintainability of the code in general (one reason which is code coupling). Would you be able to look into modifying the algorithm in the appropriate project to include your performance enhancement (as opposed to the text manipulation that you're proposing in these changes)? If anything, I think your idea of adding the checkboxes to allow client to enable/disable the feature is a good one. Let me know what your thoughts are--perhaps there's a constraint that I'm not aware of. Thanks! |
|
Hi Alan, I agree that the string manipulation approach is far from optimal. I have not been able to find the sources for Microsoft.Data.Service.Design.dll – I don’t think it have been open sourced. If you can share the sources for this project I will have a look at porting the required changes. |
|
@uffelauesen, I spent time reviewing your changes and alternatives. I was not able to find the source for Microsoft.Data.Services.Design.dll either (both public and internal searches); it does not appear to be on NuGet and I see that the binary is being referenced from within the project repo itself. I think looking for the original source will be fairly difficult. Having said that, I think the performance enhancement is a very good proposal but due to the nature of the change that we would have to make (i.e. modifying the generated output), we won't be accepting the PR at this time. From the time and effort that you put into this PR, I would urge you to keep the fork with your changes in case other users come across the same scenario as you. Should we gain more traction on this performance issue, we can certainly re-visit the topic. Thanks again for your contribution and support. |
|
Closing for now per comments. |
This PR fixes issues #45 and #44
This PR adds two new options (checkboxes) to the V1-3 advanced codegen options.
For V4 - it should be even simpler to do as you have the T4 codegen approach.