Use the same codepath for computing light-bulb actions to determine if hte lightbulb shoudl appear at all. #59599
Conversation
|
|
||
| static SuggestedActionsSourceProvider() | ||
| { | ||
| // Ensure that the list of orderings on this type matches the set we expose in s_orderings |
There was a problem hiding this comment.
Thank you for adding this!
| @@ -102,13 +102,7 @@ private partial class AsyncSuggestedActionsSource : SuggestedActionsSource, IAsy | |||
| // Collectors are in priority order. So just walk them from highest to lowest. | |||
There was a problem hiding this comment.
should be viewed with whitespace off.
| @@ -73,72 +71,87 @@ internal partial class CodeFixService : ICodeFixService | |||
| _configurationProvidersMap = GetConfigurationProvidersPerLanguageMap(configurationProviders); | |||
| } | |||
|
|
|||
| public async Task<FirstDiagnosticResult> GetMostSevereFixableDiagnosticAsync( | |||
| Document document, TextSpan range, CancellationToken cancellationToken) | |||
| private Func<string, bool>? GetShouldIncludeDiagnosticPredicate( | |||
There was a problem hiding this comment.
this helper was extracted so it can be used from two entrypoints.
|
|
||
| var spanToDiagnostics = ConvertToMap(allDiagnostics); |
There was a problem hiding this comment.
the start of this is extremely similar to StreamFixesAsync. Both start similarly (though one calls TryGetDaignostics, and one called GetDiagnostics), but how they process hte remainder is different.
| // synchronously do the work at the spot. | ||
| // | ||
| // this design's weakness is that each side don't have enough information to narrow down works to do. it | ||
| // will most likely always do more works than needed. sometimes way more than it is needed. (compilation) |
There was a problem hiding this comment.
i'm not sure how valuable some of these comments are. but i'm leaving them in.
There was a problem hiding this comment.
I'm more laughing at "this is the first...design" when we've now changed this how many times? 😄
src/EditorFeatures/Core.Wpf/Suggestions/SuggestedActionsSourceProvider.cs
Outdated
Show resolved
Hide resolved
|
Will this make #40672 more difficult? |
I don't believe so. This is just unifying code we already have to be cleaner. |
|
@jasonmalinowski this is ready for review. |
| var diagnostic = await errorDiagnosticsTask.ConfigureAwait(false) | ||
| ?? await otherDiagnosticsTask.ConfigureAwait(false); | ||
| var collection = await errorFixTask.ConfigureAwait(false) ?? | ||
| await otherFixTask.ConfigureAwait(false); | ||
| linkedTokenSource.Cancel(); |
There was a problem hiding this comment.
imo, this part of the design is unfortunate, though i get why we have it. we basically want to return an error if we have one so that the lightbulb can be red not yello. However, that means that even if we determine that we have some other fix first, we can't quickly update the UI.
| /// first. This will also attempt to return a fix for an error first, but will fall back to any fix if that | ||
| /// does not succeed. | ||
| /// </summary> | ||
| Task<FirstFixResult> GetMostSevereFixAsync(Document document, TextSpan range, CodeActionRequestPriority priority, CodeActionOptions options, CancellationToken cancellationToken); |
There was a problem hiding this comment.
outside of addOperationScope (which is used for updating the TWD progress bar for synchronous lightbulbs) this sig now matches the above.
|
@jasonmalinowski @mavasani this is ready for review. |
|
|
||
| namespace Microsoft.CodeAnalysis.CodeFixes | ||
| { | ||
| internal readonly struct FirstDiagnosticResult | ||
| internal readonly struct FirstFixResult |
| .OfType<SuggestedActionPriorityAttribute>() | ||
| .ToImmutableArray(); | ||
| Assert.Equal(attributes.Length, SuggestedActionsSourceProvider.Orderings.Length); | ||
| AssertEx.SetEqual(attributes.Select(a => a.Priority), SuggestedActionsSourceProvider.Orderings); |
There was a problem hiding this comment.
The "SetEqual" here means the orderings can be different -- is that expected here? I would have assumed "orderings" were intended to be ordered here?
There was a problem hiding this comment.
i have no idea if attributes are ordered when either emitting or reading back from metadata. So this is a setequals as we don't actually care if eitehr is true. All w care about is that the set we exported matches the items in the array in that field.
src/EditorFeatures/Test/SuggestedActions/SuggestedActionSourceProviderTests.cs
Outdated
Show resolved
Hide resolved
| { | ||
| foreach (var diagnostic in diagnostics) | ||
| { | ||
| var otherMap = diagnostic.Severity == DiagnosticSeverity.Error |
There was a problem hiding this comment.
Hmm I would have assumed "otherMap" was the "the map that doesn't match the severity", but I guess not? Or is this a typo?
There was a problem hiding this comment.
yes. that name was terrible. changed to preferredMap.
Followup to #59585