search

OData / lab

This repository is for exploring new ideas and developing early prototypes of various OData stacks.
Other
48 stars 59 forks source link

V3: Option to have lazy DSC<> initialization. Option to exclude static model #46

Closed uffelauesen closed 7 years ago

uffelauesen commented 8 years ago

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.

msftclas commented 8 years ago

Hi @uffelauesen, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

uffelauesen commented 7 years ago

Some "build trigger has fired" - I can't access the Details, if I need to change something please let me know.

AlanWong-MS commented 7 years ago

@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:

var errors = generator.GenerateCode(reader, writer, this.ServiceConfiguration.NamespacePrefix);

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!

uffelauesen commented 7 years ago

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.

AlanWong-MS commented 7 years ago

@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.

AlanWong-MS commented 7 years ago

Closing for now per comments.