Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

c-sharp service emitter #2458

Closed
wants to merge 2 commits into from
Closed

Conversation

markcowl
Copy link
Contributor

No description provided.

@github-actions
Copy link
Contributor

Changes in this PR will be published to the following url to try(check status of TypeSpec Pull Request Try It pipeline for publish status):
Playground: https://cadlplayground.z22.web.core.windows.net/prs/2458/

Website: https://tspwebsitepr.z5.web.core.windows.net/prs/2458/

@markcowl
Copy link
Contributor Author

Companion PR in azure repo here: https://github.com/Azure/typespec-azure/pull/3617

@Petermarcu
Copy link
Member

@connorjs this is the service codegen I was telling you about.

@bterlson what needs to happen to get this merged so people can try it out and give feedback?


class CSharpCodeEmitter extends CodeTypeEmitter {
#names: string[] = [];
#licenseHeader = `// Copyright (c) Microsoft Corporation. All rights reserved.
Copy link
Contributor

@connorjs connorjs Sep 27, 2023

Choose a reason for hiding this comment

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

(Curious) I understand the direct boilerplate types and the generator code itself falling under the MIT license and the Microsoft copyright.

But does this license header end up on “my” generated files from my TypeSpec? (Edit: Yes it does per my reading of the code.) I would expect that the types generated would NOT include this license header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good point, we should make this configurable so that generated code could fall under any selected license (or have no license header).

return this.emitter.result.declaration(
enumName,
code`${this.#licenseHeader}
// <auto-generated />
Copy link
Contributor

@connorjs connorjs Sep 27, 2023

Choose a reason for hiding this comment

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

(Food for thought)

  1. I often see a "generated by XXX tool" and sometimes a timestamp for generated code. Does this <auto-generated /> tag get replaced with additional detail or is this a raw comment string?

  2. I am still learning C#, but does the GeneratedCodeAttribute apply to the use case here? (In Java world, I used @Generated IIRC.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The primary reason for this is that they are keys for many c-sharp linting rules

@connorjs
Copy link
Contributor

Thanks for sharing @Petermarcu! I will try it out when released. If it does not make the October cut, I guess I could build from source and try it that way. 👍🏻

@davidfowl
Copy link

Is there an example of the output? Does it support both minimal APIs and Controllers?

cc @captainsafia

@connorjs
Copy link
Contributor

connorjs commented Oct 1, 2023

+1 to seeing a full example. (Maybe an "end to end" test can reference a .cs file that provides the example?)

For now, I scanned through the diff. service.ts contains the core logic and gave me a decent idea of output.

@markcowl
Copy link
Contributor Author

markcowl commented Oct 7, 2023

@connorjs Yes, the best end-to-end at this point is an Azure-specific end-to-end. Adding a REST end-to-end equivalent in samples and in the unit tests is on the task list for check-in (hopefully next week).

@markcowl
Copy link
Contributor Author

markcowl commented Oct 7, 2023

@davidfowl The plan is to allow modelsOnly or modelsAndOperations or full asp.net with controllers, based on configuration. For the first iteration, it is the whole thing.

enumName,
code`${this.#licenseHeader}
// <auto-generated />
using Newtonsoft.Json;
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason System.Text.Json isn't used?

Copy link
Contributor Author

@markcowl markcowl Oct 26, 2023

Choose a reason for hiding this comment

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

We have an issue tracking that in the Azure repo here: https://github.com/Azure/typespec-azure/issues/3285. The thought is that we will use the same serialization mechanism as Azure client SDKs, which is built on core .net libraries, but does not primarily use System.Text.Json (also does not use Newtonsoft.Json).

I will recreate the general interest issues in this repo so folks can follow along

@connorjs
Copy link
Contributor

Do y’all have any planned timelines around merging this PR and releasing it? Is it possible to get this into the November release?

@connorjs
Copy link
Contributor

Hi again. I’m excited to see the November release of TypeSpec but know this did not make it. I also know we are entering Holiday season where development like this likely slows down.

Please let me know if I can help with anything (reviews, beta testing, documentation, sample repos, etc). I would love to pull this into our repositories in December/early January, so that I can reference it in beginning of the year discussions and planning.

@jlarmstrongiv jlarmstrongiv mentioned this pull request Apr 6, 2024
3 tasks
@markcowl markcowl changed the title Move service emitter to oss repo c-sharp service emitter Jul 22, 2024
@markcowl markcowl closed this Jul 22, 2024
@connorjs
Copy link
Contributor

For anyone else watching C# Service emitter, I personally will just look at the related label I see: emitter:service:csharp

Happy to be corrected on this. Just very excited to see the progress 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants