-
Notifications
You must be signed in to change notification settings - Fork 10k
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 codefixer and completion provider to install OpenAPI package from extension methods #55963
Conversation
d283852
to
411d738
Compare
src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Dependencies/ThisAndExtensionMethod.cs
Outdated
Show resolved
Hide resolved
24a6324
to
ab4bbe6
Compare
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
This seems like a good general purpose fixer, do we plan on adding more of our methods to this? |
We could -- there'd be some additional work to do to source all of the APIs that could be invoked here and provide them to this extension point in some way. For expediency, I just focused on the APIs in the OpenAPI surface area here. I can file an issue to track making this more general purpose. See #56092. |
src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Dependencies/AddPackageFixer.cs
Outdated
Show resolved
Hide resolved
} | ||
|
||
var targetThisAndExtensionMethod = new ThisAndExtensionMethod(symbolType, methodName); | ||
if (_wellKnownExtensionMethodCache.TryGetValue(targetThisAndExtensionMethod, out var packageSourceAndNamespace)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a dictionary worthwhile for 2 members, or do we anticipate more in the near future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I anticipate that we'll have a larger collection here (see #56092). Whether or not Dictionary
ends up being the right type for the complete set of extension methods remains to be seen but it seemed sufficient for now.
src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Dependencies/AddPackageFixer.cs
Outdated
Show resolved
Hide resolved
No could, we should. What is the work to make this work for all of the ASP.NET Core extension methods? |
Thanks for the grammar lesson. 🙃 #56092 tracks doing this for everything. Before we do that, we should pilot the package install fixer and a completion provider to recommend extension methods from external packages on primary types with the OpenAPI surface area. As far as genericizing/automating this, we'll probably need to author some sort of script to scrape the metadata the analyzer consumes from the repo and feed it through. |
@IEvangelist Random question, what do the docs do to get this list https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.builder.iapplicationbuilder?view=aspnetcore-8.0#extension-methods? I am thinking maybe we can use a similar process to populate the list of methods and their packages. |
I'm not sure how familiar you are with all of this, so I'll speak as if you're not familiar with any of it. This is something that is generated by the DocFx, specifically for the API docs. It uses a process similar to the following:
But that's something that specific to API docs, not the conceptual content that I believe you're referring to. Do you like the format of the extensions, or what exactly are you thinking? Is the idea to generate reference content for extensions and then map their NuGet packages in the table? Also, I assume you're interested in something like this for .NET Aspire? |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JamesNK Can I get your review on this given your experience implementing completion providers in route tooling?
The goal is to add completions for extension methods defined in a separate package then provide an installation codefix for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been a minute since my head was in The Roslyn Zone. I'll do my best.
src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Dependencies/AddPackageFixer.cs
Show resolved
Hide resolved
src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Dependencies/AddPackageFixer.cs
Show resolved
Hide resolved
src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Dependencies/AddPackageFixer.cs
Show resolved
Hide resolved
src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Dependencies/AddPackageFixer.cs
Show resolved
Hide resolved
|
||
[ExportCompletionProvider(nameof(ExtensionMethodsCompletionProvider), LanguageNames.CSharp)] | ||
[Shared] | ||
public sealed class ExtensionMethodsCompletionProvider : CompletionProvider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment summarizing what the provider's deal is.
Does it replace completion for known types or are values from the completion provider added to the auto-complete? Replacing auto-complete for some types (if that's how it works) feels pretty invasive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The completion provider is additive (assuming I understand the intent of the context.AddItem
method 😅 ) and is intended to augment the completions list with methods that we think might be helpful. I'll add a comment to this effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the value is already present in auto-complete? For example, someone might already have a reference to the OpenAPI package. Do they see the AddOpenApi
option from built-in autocomplete and a item from this provider?
Do items added by this provider look the same or different to other items? Could I see a screenshot?
|
||
[ExportCompletionProvider(nameof(ExtensionMethodsCompletionProvider), LanguageNames.CSharp)] | ||
[Shared] | ||
public sealed class ExtensionMethodsCompletionProvider : CompletionProvider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So excited! 🥳
src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Dependencies/ThisAndExtensionMethod.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. One small change suggested
Screen.Recording.2024-06-14.at.11.16.18.AM.mov