Use new UI for this./Me. and add a fix-all code action for qualification.
#11227
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brettfo
added
Area-IDE
PR For Personal Review Only
brettfo
removed
the
PR For Personal Review Only
|
@dotnet/roslyn-ide for review |
brettfo
changed the title
this./Me. and add a fix-all code action for qualification.this./Me. and add a fix-all code action for qualification.
| @@ -33,19 +34,46 @@ internal abstract class SimplifyTypeNamesDiagnosticAnalyzerBase<TLanguageKindEnu | |||
| customTags: DiagnosticCustomTags.Unnecessary); | |||
|
|
|||
| private static readonly LocalizableString s_localizableTitleRemoveThisOrMe = new LocalizableResourceString(nameof(FeaturesResources.RemoveQualification), FeaturesResources.ResourceManager, typeof(FeaturesResources)); | |||
| private static readonly DiagnosticDescriptor s_descriptorRemoveThisOrMe = new DiagnosticDescriptor(IDEDiagnosticIds.RemoveQualificationDiagnosticId, | |||
| private static readonly DiagnosticDescriptor s_descriptorRemoveThisOrMeHidden = new DiagnosticDescriptor(IDEDiagnosticIds.RemoveQualificationDiagnosticId, | |||
There was a problem hiding this comment.
If you look at the naming styles stuff, I think you can have a single DiagnosticDescriptor and then return a different severity later (Just be careful, if your "defaultSeverity" is Hidden, then the anlayzer won't be run on closed files).
|
👍 Anyone else from @dotnet/roslyn-ide? |
brettfo
force-pushed
the
prefer-this-notifications
branch
from
f8a87ca
to
386a406
Compare
|
Rebased to fix merge conflicts. If nobody else has any complaints imma merge. Not really; I'm just trying to scare some reviews out of people. |
brettfo
force-pushed
the
prefer-this-notifications
branch
from
386a406
to
fe5dace
Compare
|
test vsi please |
brettfo
force-pushed
the
prefer-this-notifications
branch
from
fe5dace
to
65431ed
Compare
|
FYI, the internal component of this PR is dotnet/roslyn-internal#1000. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
High Level
This PR updates the
this./Me.code style options to support the requested notification levels (None,Info,Warning,Error) and also enables a fix-all code action.Details
this./Me.live.SimpleCodeStyleOptionwas renamed toCodeStyleOption<T>to allow for any option value type, although onlyboolis currently used.Contentious points
SimplificationOptions.QualifyFieldAccess) were obsoleted in favor of the new options (e.g.,CodeStyleOptions.QualifyFieldAccess) because the old options were only simplebools, but the new ones also manage their notification level. With this obsoletion, do we need to provide a way for the old option to migrate forward? Or since we're targeting a future release of VS can we simply delete the old options from the API? If we have to support the old options still, what should happen to the notification preference if I set the old option value tofalse. Does that meanNotificationOptions.NoneorErroror something more complicated?SimplificationOptions.QualifyFieldAccess, etc. and supplementing them with additional options likeCodeStyleOptions.QualifyFieldAccessNotification, but that feels weird for two reasons, 1) the options are coupled in the UI but de-coupled in the API; what's the mechanism for discoverability between the two? And 2) the existingvar/keyword, etc., options are already coupled together and the inconsistency might kill me.Still pending