Add CustomTags to CodeAction#51456
Conversation
| /// <summary> | ||
| /// Gets custom tags for the CodeAction. | ||
| /// </summary> | ||
| public virtual ImmutableArray<string> CustomTags => ImmutableArray<string>.Empty; |
There was a problem hiding this comment.
does this need to be public?
There was a problem hiding this comment.
I think in the design meeting we decided that we will expose this as a public API, is that correct @jasonmalinowski? Also, if we are exposing this public property, we should also expose new Code.Create overloads that take custom tags argument.
There was a problem hiding this comment.
It looks like we discussed this on the 12/7/2020 design meeting; I don't recall the details (and the conclusion from the written notes isn't clear), but my memory matches @mavasani's in that we were OK making a public API like this. Especially since we already have one on Diagnostic and have good experience with it.
src/EditorFeatures/TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs
Outdated
Show resolved
Hide resolved
|
I don't see what code is actually populating this. |
Yup, that is listed as a todo in the description. Wanted to open this in its current state to get feedback. Updating each provider to set this will take me a bit. |
|
Can you show a single example? That would be helpful. |
Sure, I welcome ideas on how to do this more efficiently. private sealed class MyCodeAction : CustomCodeActions.DocumentChangeAction
{
public MyCodeAction(Func<CancellationToken, Task<Document>> createChangedDocument)
- : base(CSharpAnalyzersResources.Add_braces, createChangedDocument, CSharpAnalyzersResources.Add_braces)
+ : base(CSharpAnalyzersResources.Add_braces, createChangedDocument, CSharpAnalyzersResources.Add_braces, customTags: ImmutableArray.Create(PredefinedCodeFixProviderNames.AddBraces))
{
}
} |
What probably makes more sense is to require CustomCodeActions.DocumentChangeAction (and its peers) to take in the actual provider instance that created them. Then in the base type, just grab whatever data we needed off of that provider and store that in the code action. |
|
Basically, we would an approach that would force us to remember to do this in all cases. So forcing the clients to always pass in themselves when creating these special code action subclasses would be good way to enforce that. |
Great idea as most providers already set this in their ExportCodeFixProvider attribute. So glad I opened this PR early. Thanks =) |
|
I'd also be fine with us just grabbing the type name of provider. That way we can work across all providers. |
I may do that as a fallback if the Name property isn't set on the attribute. There were concerns about renames breaking partners who may take dependeny on this information. |
Er... what partners would those be? I would not ever want to make this into a contract. We definitely may change impls/refactor/etc. there. what are we trying to use this for? If we know that, it can help us drive what an appropriate solution is here. |
Razor will be using these identifiers to help process Code Actions within the Razor Language Server. I believe there was also a telemetry use case for this (not sure of the specifics on this though). |
There was a problem hiding this comment.
These are not in the same style as previous Names. Would be happy to standardize on either format if requested.
There was a problem hiding this comment.
Presumably these aren't displayed anywhere?
There was a problem hiding this comment.
As in they just use nameof instead spacing? I think it's fine personally. These are not customer facing names, just identifiers for us to use and read.
There was a problem hiding this comment.
Yeah, I figured they weren't displayed anywhere too exciting since they're not localized, but I thought maybe they were possibly shown in a yellow bar, where localization would be annoying, but perhaps spaces were the convention.
a73b0f6 to
c61f64b
Compare
db17d9c to
b733708
Compare
src/EditorFeatures/Test/CodeActions/CodeChangeProviderMetadataTests.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/Test/CodeActions/CodeChangeProviderMetadataTests.cs
Outdated
Show resolved
Hide resolved
ryzngard
left a comment
There was a problem hiding this comment.
Requesting changes to clarify behavior in this comment https://github.com/dotnet/roslyn/pull/51456/files#r599268301
Once we decide on that I think everything else looks good
Part of the work to support #4919
Adds a CustomTags property to CodeActions which will be included as part of the CodeActionResolveData.