-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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 'priority' concept to our code actions, and code refactoring/fix providers. #68511
Changes from 11 commits
7b9dab4
8b175b0
72795ac
be0075a
f00a244
38a8d57
0ac048a
8906650
1a6240e
657bf1f
5342a43
ff293b1
b92df55
ff2b894
cb2ae1b
741e7ad
81a4645
704b40e
8b8f3cc
38e021c
aff8526
939e28b
7398b34
b33d117
1ca450b
4e24505
3ac6bc1
974cf63
ce1286b
9abc31e
9334477
0624db5
bdecd94
685ed9d
37a2b3f
178a730
e2a04c7
bdbf75e
82a9e30
11d72c1
0d59cca
d95498b
39ed273
923eac7
b9cb2ab
7da1751
71b1862
c7f16db
dcb6052
138d3a3
81458d9
6d5c41b
605ee29
773e21c
76e235b
024a103
8ced823
72f86df
ff02e51
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,7 +40,7 @@ protected AbstractBuiltInCodeStyleDiagnosticAnalyzer(ImmutableArray<DiagnosticDe | |
Debug.Assert(!supportedDiagnostics.Any(descriptor => descriptor.CustomTags.Any(t => t == WellKnownDiagnosticTags.Unnecessary)) || this is AbstractBuiltInUnnecessaryCodeStyleDiagnosticAnalyzer); | ||
} | ||
|
||
public virtual CodeActionRequestPriority RequestPriority => CodeActionRequestPriority.Normal; | ||
public virtual CodeActionRequestPriorityInternal RequestPriority => CodeActionRequestPriorityInternal.Normal; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we could consider makign the diagnostic-analyzer priority part available as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❓ Can we keep the type of this property |
||
public sealed override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } | ||
|
||
protected static DiagnosticDescriptor CreateDescriptorWithId( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,8 +59,8 @@ protected AbstractResolveConflictMarkerCodeFixProvider( | |
/// them if they bring up the lightbulb on a <c><<<<<<<</c> line, it should run ahead of | ||
/// normal fix providers else so the user can quickly fix the conflict and move onto the next conflict. | ||
/// </summary> | ||
private protected override CodeActionRequestPriority ComputeRequestPriority() | ||
=> CodeActionRequestPriority.High; | ||
private protected override CodeActionRequestPriorityInternal ComputeRequestPriorityInternal() | ||
=> CodeActionRequestPriorityInternal.High; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. features that return 'high' use the 'internal' priority system. For now, only we allow a blessed set to be high pri. That set is exactly:
|
||
|
||
#endif | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. providing priority is no longer something CODE_STYLE cannot do. |
||
|
||
|
@@ -301,7 +301,7 @@ static CodeAction CreateCodeAction(string title, Func<CancellationToken, Task<Do | |
#if CODE_STYLE | ||
return CodeAction.Create(title, action, equivalenceKey); | ||
#else | ||
return CodeAction.DocumentChangeAction.Create(title, action, equivalenceKey, CodeActionPriority.High); | ||
return CodeAction.DocumentChangeAction.Create(title, action, equivalenceKey, CodeActionPriorityInternal.High); | ||
#endif | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. however, because it is high pri, the feature has to use this backdoor for that to be ok. it will be clamped otherwise. |
||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -30,8 +30,8 @@ public sealed override ImmutableArray<string> FixableDiagnosticIds | |||||||||||
/// them acting on an error reported in code, and can be computed fast as it only uses syntax not semantics. | ||||||||||||
/// It's also the 8th most common fix that people use, and is picked almost all the times it is shown. | ||||||||||||
/// </summary> | ||||||||||||
private protected override CodeActionRequestPriority ComputeRequestPriority() | ||||||||||||
=> CodeActionRequestPriority.High; | ||||||||||||
private protected override CodeActionRequestPriorityInternal ComputeRequestPriorityInternal() | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, this API/override is still not available in the code style layer. This means the formatting analyzer/fixer when executed from the CodeStyle NuGet package would still be clamped down to normal priority. I am wondering if there is some way we can incorporate that support in this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am wondering if the following implementation could work here:
Agreed that this opens a small backdoor for third party providers to now add this special custom tag themselves and trick us into thinking it's a first party provider, but I think such providers will be rare, if any. On the positive side though, we avoid all this overengineering with parallel Internal and Public APIs, and can finally have our first party providers shipping in CodeStyle NuGet package to also be able to run at high priority when appropriate. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. I'll do that! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Slightly confused though. If this is not available, why would this Compile There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ignore that. I see the ifdef :-) |
||||||||||||
=> CodeActionRequestPriorityInternal.High; | ||||||||||||
|
||||||||||||
#endif | ||||||||||||
|
||||||||||||
|
@@ -52,7 +52,7 @@ public sealed override Task RegisterCodeFixesAsync(CodeFixContext context) | |||||||||||
AnalyzersResources.Fix_formatting, | ||||||||||||
c => FixOneAsync(context, diagnostic, c), | ||||||||||||
nameof(AbstractFormattingCodeFixProvider), | ||||||||||||
CodeActionPriority.High), | ||||||||||||
CodeActionPriorityInternal.High), | ||||||||||||
diagnostic); | ||||||||||||
#endif | ||||||||||||
} | ||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,7 +64,7 @@ internal SuggestedAction( | |
CodeAction = codeAction; | ||
} | ||
|
||
internal virtual CodeActionPriority Priority => CodeAction.Priority; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. never used. removed. |
||
internal virtual CodeActionPriorityInternal Priority => CodeAction.PriorityInternal; | ||
|
||
public virtual bool TryGetTelemetryId(out Guid telemetryId) | ||
{ | ||
|
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.
Lowest/Low/Default/High/Highest matches editor naming for this sort of thing (including the lightbulb priority class namign. so i aligned on that for our public API).
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 also preferred 'Default' (versus 'Normal' or 'Medium') as it prevented the need to have a duplicate 'Default' member, or to explain what the 'Default' value for these enums was. By just naming it that way, it all simplified (though it does impact the diff here).