Skip to content

Allow making generated types internal to hide them outside the assembly - #100

Closed
habbes wants to merge 25 commits into
OData:masterfrom
habbes:93-generate-internal-types
Closed

Allow making generated types internal to hide them outside the assembly#100
habbes wants to merge 25 commits into
OData:masterfrom
habbes:93-generate-internal-types

Conversation

@habbes

@habbes habbes commented Jan 16, 2020

Copy link
Copy Markdown
Contributor

Addresses issue #93, #97, #101

  • Add an option in the advanced settings UI configuration of the service that makes the generated types (classes and enums) internal
  • Add support for VS 2019
  • Added some tests to the project, including testing some of the pre-existing components
  • Add azure-pipelines.yml build pipeline

This PR handles the "internal" option for both C# and VB generated code. However it appears the connected service extension is not enabled for VB projects.

PS: I did not test this against OData V3 services.

@habbes
habbes marked this pull request as ready for review January 17, 2020 07:49
@KanishManuja-MS

KanishManuja-MS commented Jan 18, 2020

Copy link
Copy Markdown
Contributor

@habbes The changes look fine to me. The only thing I would ask is to add one or more test cases for this new configuration. I will do a more in-depth code review and approve.

I think this would be supported for v3 for your changes as well, so just do a quick check.
However, if it does not work, I would be fine with deprecating support for v3 (will need wider consensus) for new features but I would probably want to create a new extension (and rename old one) on the market place, so extension for v3 is still available.

@habbes

habbes commented Jan 20, 2020

Copy link
Copy Markdown
Contributor Author

@KanishManuja-MS is there a sample V3 service endpoint I can test with?

I had created an issue (#101) to add tests to the project on a separate PR because it does not have any testing at all. But let me just go ahead and create a test project for this PR as you have advised.

Comment thread ODataConnectedService/src/Templates/ODataT4CodeGenerator.ttinclude Outdated

@paulodero paulodero left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we already have tests for OData Code generator and connected service is simply an extension of OData code Generator, I don't think it is a good idea to add a whole new test solution. I would rather figure out how to reuse the OData code generator tests and build on top of it. Since we may deprecate one of the tools, this may leave us in a good place when that happens.

@habbes

habbes commented Jan 21, 2020

Copy link
Copy Markdown
Contributor Author

@paulodero

Since we already have tests for OData Code generator and connected service is simply an extension of OData code Generator, I don't think it is a good idea to add a whole new test solution. I would rather figure out how to reuse the OData code generator tests and build on top of it. Since we may deprecate one of the tools, this may leave us in a good place when that happens.

I think we would still need to create a test solution for this. There are things that are ODataConnectedService specifc that would still need to be tested that we can't just copy from the code generator (i.e. the VS Connected Services related stuff). And for the parts that are common, it is possible that some functionality has diverged over time. I think manually copying the tests from the Client code gen project to the code generator portion of this project could help save time, and it might present opportunities for us to refactor and make the code gen tests easier to work with instead of copying them as is. And if this library does not support VB, then we would be adding extra weight if we just used the code gen tests as-is.

@paulodero paulodero left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to ensure we do not rewrite already existing test cases. Its the same T4 templates. We would rather build on top of it and refactor where necessary.

Comment thread ODataConnectedService/src/Templates/ODataT4CodeGenerator.cs Outdated
@habbes habbes added the ready for review Use this label if you want your pull request to be reviewed label Jan 23, 2020
@habbes

habbes commented Jan 23, 2020

Copy link
Copy Markdown
Contributor Author

@KanishManuja-MS

I've found a V3 endpoint to test with (https://services.odata.org/V3/(S(j4ofwcgyinw2fuqmqz4jgmuy))/OData/OData.svc/). This new configuration does not support V3. V3 uses a different code generator, namely the System.Data.Services.Design.EntityClassGenerator from the Microsoft.Data.Services.Design assembly, which is not part of the source code. So we can't add new configuration to it to change how it generates code. I've gone ahead and removed that option from setup wizard when a V3 endpoint is provided.

I've added a couple of test cases to test the code generation as well as general test cases for some of the main components of the connected service. I think this could do for now but we should try to improve overall test coverage in future PR's.

Furthermore, the tests should be added to the build pipeline, is it something I can do from my end, and if so, could you point me in the right direction?

Comment thread ODataConnectedService/src/Templates/ODataT4CodeGenerator.ttinclude Outdated

@paulodero paulodero left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add the tests to the build pipeline. A good opportunity to learn how to do it.

@paulodero paulodero left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be good to synchronize with already existing tests in code gen and add the tests to the build pipeline.

@habbes
habbes force-pushed the 93-generate-internal-types branch from 3dd1260 to bdf43ed Compare February 5, 2020 07:14
@habbes

habbes commented Feb 25, 2020

Copy link
Copy Markdown
Contributor Author

A branch with the same changes as the ones from this PR were already merged to the repo (see PR #107), therefore we can close this PR.

@habbes habbes closed this Feb 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for review Use this label if you want your pull request to be reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants