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

Final naming updates for ModelReaderWriter #40766

Merged
merged 5 commits into from
Dec 14, 2023
Merged

Conversation

m-nash
Copy link
Member

@m-nash m-nash commented Dec 13, 2023

Addresses the following comments from apiview

Contributing to the Azure SDK

Please see our CONTRIBUTING.md if you are not familiar with contributing to this repository or have questions.

For specific information about pull request etiquette and best practices, see this section.

update name of JsonModelConverter
@m-nash m-nash requested a review from tg-msft as a code owner December 13, 2023 19:57
@azure-sdk
Copy link
Collaborator

azure-sdk commented Dec 13, 2023

API change check

APIView has identified API level changes in this PR and created following API reviews.

Azure.Core
System.ClientModel

@annelo-msft
Copy link
Member

Super nit - the links to APIView don't resolve once the comments are closed, so it's hard to understand from the links in the description the comments are addressing. Probably an APIView issue/bug, but until it's resolved, it'd be helpful to clarify what's happening in the PR description

@annelo-msft
Copy link
Member

Another nit - possible APIView bug - the ClientModel APIView doesn't look like it's getting linked from the PR comment: #40766 (comment)
Do we know why? It'd be easier to review the PR if there was an APIView linked.

Copy link
Member

@annelo-msft annelo-msft left a comment

Choose a reason for hiding this comment

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

I like the naming unifications @tg-msft is driving for, but I'm on board with whatever architects decide. Thanks for making this happen, @m-nash!

@m-nash m-nash merged commit 92ea9b3 into main Dec 14, 2023
@m-nash m-nash deleted the mnash-moveMrwNamespace branch December 14, 2023 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants