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

Light up System.ClientModel interface decorations on public API surface #8386

Merged

Conversation

christothes
Copy link
Member

@christothes christothes commented Jun 6, 2024

fixes #8326
This change adds allowed .nupkg dependencies to the compilation as references so that the compilation has the full metadata for interfaces that live in those dependencies. Prior to this change, interfaces in referenced assemblies such as System.ClientModel would never show up as public APIs for types that implement them.

Azure.Core.RehydrationToken before:
image

after: (results from local testing):
image

@christothes christothes self-assigned this Jun 7, 2024
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.

This looks great! (From the "after" screenshot)

Will it work for all .NET packages? i.e. will IJsonModel<T> and IPersistableModel<T> start showing up for clients as well -- not just Azure.Core?

Many thanks!

@christothes
Copy link
Member Author

Will it work for all .NET packages? i.e. will IJsonModel<T> and IPersistableModel<T> start showing up for clients as well -- not just Azure.Core?

Good question - I will need to test this since the current behavior change will interrogate the dependencies for the primary nupkg being reviewed. For clients, I think they would only depend on Azure.Core, which then depends on System.ClientModel. So I may need to recursively follow dependencies for it to work with other clients.

If you could point me to an existing client that implements a type that would be decorated with a System.ClientModel interface, I can validate.

@annelo-msft
Copy link
Member

If you could point me to an existing client that implements a type that would be decorated with a System.ClientModel interface, I can validate.

Looks like Azure.AI.OpenAI and Azure.Security.CodeTransparency both do.

@christothes
Copy link
Member Author

Looks like clients take a direct dependency on System.ClientModel so we should be good!

Copy link
Member

@m-nash m-nash left a comment

Choose a reason for hiding this comment

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

The imagine showing the apiview has an expansion of the interfaces which I am not sure we want.

For instance it shows IJsonModel<T> and IPersitableModel<T>. The actual class only specifies IJsonModel<T> https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/Azure.Core/src/RehydrationToken.Serialization.cs#L11.

I do see that we do this with IEnumerable<T> expanding it to IEnumerable, but just wondering if we really want this as it the very first thought I had was why are we explicitly specifying IPersistableModel<T> but I had to dig into the code to find out we don't.

@christothes christothes merged commit de379c3 into Azure:main Jul 5, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Include interfaces from System.ClientModel in APIView
4 participants