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

Add @useSystemTextJsonConverter decorator for csharp #1336

Merged
merged 29 commits into from
Aug 9, 2024

Conversation

live1206
Copy link
Member

@live1206 live1206 commented Aug 9, 2024

Resolves #1229
This change has already been approved in #1230

@azure-sdk
Copy link
Collaborator

azure-sdk commented Aug 9, 2024

All changed packages have been documented.

  • @azure-tools/typespec-client-generator-core
Show changes

@azure-tools/typespec-client-generator-core - feature ✏️

Add @hasJsonConverter for csharp only to indicate if JSON converter is needed

@azure-sdk
Copy link
Collaborator

You can try these changes here

🛝 Playground 🌐 Website 📚 Next docs

@tadelesh tadelesh merged commit 5b46eef into Azure:release/august-2024 Aug 9, 2024
28 checks passed
@live1206 live1206 deleted the json-converter branch August 9, 2024 06:35
@iscai-msft
Copy link
Contributor

@live1206 @tadelesh @srnagar I thought the plan here was to make sure this decorator would also work for Java. Based off of the name of the decorator, it still seems to be csharp specific

@timotheeguerin
Copy link
Member

timotheeguerin commented Aug 9, 2024

yeah made the same comment on the old PR #1230 (comment)

My opinion is that either

  1. this is a very specific csharp decorator like the name implies here and tghen we should not add scope and put in a CSharp sub namespace
  2. Make it generic enough that other language might use it and then use the scoping system

@live1206
Copy link
Member Author

@live1206 @tadelesh @srnagar I thought the plan here was to make sure this decorator would also work for Java. Based off of the name of the decorator, it still seems to be csharp specific

This decorator does not fit the usage for Java, please check https://teams.microsoft.com/l/message/19:meeting_MGQ4YTVhYTYtZWU1OC00NzI4LWEzYzktMjgzYzg3OTBkY2Q5@thread.v2/1723169831963?context=%7B%22contextType%22%3A%22chat%22%7D

I raised the concern of csharp specific decorator, but we decided to go in root namespace and default to all scopes during the previous discussion, please check https://teams.microsoft.com/l/message/19:meeting_MGQ4YTVhYTYtZWU1OC00NzI4LWEzYzktMjgzYzg3OTBkY2Q5@thread.v2/1723177908738?context=%7B%22contextType%22%3A%22chat%22%7D

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