From 6614768e064725c9a881726ec376a69dd5b75939 Mon Sep 17 00:00:00 2001 From: Manish Vasani Date: Tue, 14 Jan 2020 17:41:57 -0800 Subject: [PATCH] Address PR feedback --- .../DeclarationNameCompletionProviderTests.cs | 80 +++++++------------ .../PartialMethodCompletionProviderTests.cs | 21 ++--- .../CSharpTest/Formatting/CodeCleanupTests.cs | 3 +- .../Formatting/FormattingEngineTestBase.cs | 7 +- .../CSharpTest/Workspaces/WorkspaceTests.cs | 16 +++- .../CodeGeneration/CodeGenerationTests.cs | 6 +- .../CSharp/Impl/Options/AutomationObject.cs | 1 - 7 files changed, 56 insertions(+), 78 deletions(-) diff --git a/src/EditorFeatures/CSharpTest/Completion/CompletionProviders/DeclarationNameCompletionProviderTests.cs b/src/EditorFeatures/CSharpTest/Completion/CompletionProviders/DeclarationNameCompletionProviderTests.cs index 8aded0e6794..9e8070e341b 100644 --- a/src/EditorFeatures/CSharpTest/Completion/CompletionProviders/DeclarationNameCompletionProviderTests.cs +++ b/src/EditorFeatures/CSharpTest/Completion/CompletionProviders/DeclarationNameCompletionProviderTests.cs @@ -1265,24 +1265,16 @@ void M() public async Task DisabledByOption() { var workspace = WorkspaceFixture.GetWorkspace(); - var originalOptions = workspace.CurrentSolution.Options; - try - { - workspace.TryApplyChanges(workspace.CurrentSolution.WithOptions(originalOptions. - WithChangedOption(CompletionOptions.ShowNameSuggestions, LanguageNames.CSharp, false))); + workspace.TryApplyChanges(workspace.CurrentSolution.WithOptions(workspace.Options. + WithChangedOption(CompletionOptions.ShowNameSuggestions, LanguageNames.CSharp, false))); - var markup = @" + var markup = @" class Test { Test $$ } "; - await VerifyNoItemsExistAsync(markup); - } - finally - { - workspace.TryApplyChanges(workspace.CurrentSolution.WithOptions(originalOptions)); - } + await VerifyNoItemsExistAsync(markup); } [WorkItem(23590, "https://github.com/dotnet/roslyn/issues/23590")] @@ -1458,48 +1450,35 @@ public void Method() public async Task CustomNamingStyleInsideClass() { var workspace = WorkspaceFixture.GetWorkspace(); - var originalOptions = workspace.Options; + workspace.TryApplyChanges(workspace.CurrentSolution.WithOptions(workspace.Options.WithChangedOption( + new OptionKey(SimplificationOptions.NamingPreferences, LanguageNames.CSharp), + NamesEndWithSuffixPreferences()))); - try - { - workspace.TryApplyChanges(workspace.CurrentSolution.WithOptions(originalOptions.WithChangedOption( - new OptionKey(SimplificationOptions.NamingPreferences, LanguageNames.CSharp), - NamesEndWithSuffixPreferences()))); - - var markup = @" + var markup = @" class Configuration { Configuration $$ } "; - await VerifyItemExistsAsync(markup, "ConfigurationField", glyph: (int)Glyph.FieldPublic, - expectedDescriptionOrNull: CSharpFeaturesResources.Suggested_name); - await VerifyItemExistsAsync(markup, "ConfigurationProperty", glyph: (int)Glyph.PropertyPublic, - expectedDescriptionOrNull: CSharpFeaturesResources.Suggested_name); - await VerifyItemExistsAsync(markup, "ConfigurationMethod", glyph: (int)Glyph.MethodPublic, - expectedDescriptionOrNull: CSharpFeaturesResources.Suggested_name); - await VerifyItemIsAbsentAsync(markup, "ConfigurationLocal"); - await VerifyItemIsAbsentAsync(markup, "ConfigurationLocalFunction"); - } - finally - { - workspace.TryApplyChanges(workspace.CurrentSolution.WithOptions(originalOptions)); - } + await VerifyItemExistsAsync(markup, "ConfigurationField", glyph: (int)Glyph.FieldPublic, + expectedDescriptionOrNull: CSharpFeaturesResources.Suggested_name); + await VerifyItemExistsAsync(markup, "ConfigurationProperty", glyph: (int)Glyph.PropertyPublic, + expectedDescriptionOrNull: CSharpFeaturesResources.Suggested_name); + await VerifyItemExistsAsync(markup, "ConfigurationMethod", glyph: (int)Glyph.MethodPublic, + expectedDescriptionOrNull: CSharpFeaturesResources.Suggested_name); + await VerifyItemIsAbsentAsync(markup, "ConfigurationLocal"); + await VerifyItemIsAbsentAsync(markup, "ConfigurationLocalFunction"); } [Fact, Trait(Traits.Feature, Traits.Features.Completion)] public async Task CustomNamingStyleInsideMethod() { var workspace = WorkspaceFixture.GetWorkspace(); - var originalOptions = workspace.Options; + workspace.TryApplyChanges(workspace.CurrentSolution.WithOptions(workspace.Options.WithChangedOption( + new OptionKey(SimplificationOptions.NamingPreferences, LanguageNames.CSharp), + NamesEndWithSuffixPreferences()))); - try - { - workspace.TryApplyChanges(workspace.CurrentSolution.WithOptions(originalOptions.WithChangedOption( - new OptionKey(SimplificationOptions.NamingPreferences, LanguageNames.CSharp), - NamesEndWithSuffixPreferences()))); - - var markup = @" + var markup = @" class Configuration { void M() @@ -1508,18 +1487,13 @@ void M() } } "; - await VerifyItemExistsAsync(markup, "ConfigurationLocal", glyph: (int)Glyph.Local, - expectedDescriptionOrNull: CSharpFeaturesResources.Suggested_name); - await VerifyItemExistsAsync(markup, "ConfigurationLocalFunction", glyph: (int)Glyph.MethodPublic, - expectedDescriptionOrNull: CSharpFeaturesResources.Suggested_name); - await VerifyItemIsAbsentAsync(markup, "ConfigurationField"); - await VerifyItemIsAbsentAsync(markup, "ConfigurationMethod"); - await VerifyItemIsAbsentAsync(markup, "ConfigurationProperty"); - } - finally - { - workspace.TryApplyChanges(workspace.CurrentSolution.WithOptions(originalOptions)); - } + await VerifyItemExistsAsync(markup, "ConfigurationLocal", glyph: (int)Glyph.Local, + expectedDescriptionOrNull: CSharpFeaturesResources.Suggested_name); + await VerifyItemExistsAsync(markup, "ConfigurationLocalFunction", glyph: (int)Glyph.MethodPublic, + expectedDescriptionOrNull: CSharpFeaturesResources.Suggested_name); + await VerifyItemIsAbsentAsync(markup, "ConfigurationField"); + await VerifyItemIsAbsentAsync(markup, "ConfigurationMethod"); + await VerifyItemIsAbsentAsync(markup, "ConfigurationProperty"); } [WorkItem(31304, "https://github.com/dotnet/roslyn/issues/31304")] diff --git a/src/EditorFeatures/CSharpTest/Completion/CompletionProviders/PartialMethodCompletionProviderTests.cs b/src/EditorFeatures/CSharpTest/Completion/CompletionProviders/PartialMethodCompletionProviderTests.cs index 372ff8e3286..fc134850826 100644 --- a/src/EditorFeatures/CSharpTest/Completion/CompletionProviders/PartialMethodCompletionProviderTests.cs +++ b/src/EditorFeatures/CSharpTest/Completion/CompletionProviders/PartialMethodCompletionProviderTests.cs @@ -483,15 +483,11 @@ partial class PClass public async Task ExpressionBodyMethod() { var workspace = WorkspaceFixture.GetWorkspace(); - var originalOptions = workspace.Options; + workspace.TryApplyChanges(workspace.CurrentSolution.WithOptions(workspace.Options.WithChangedOption( + CSharpCodeStyleOptions.PreferExpressionBodiedMethods, + new CodeStyleOption(ExpressionBodyPreference.WhenPossible, NotificationOption.Silent)))); - try - { - workspace.TryApplyChanges(workspace.CurrentSolution.WithOptions(originalOptions.WithChangedOption( - CSharpCodeStyleOptions.PreferExpressionBodiedMethods, - new CodeStyleOption(ExpressionBodyPreference.WhenPossible, NotificationOption.Silent)))); - - var text = @"using System; + var text = @"using System; partial class Bar { partial void Foo(); @@ -500,7 +496,7 @@ partial class Bar " ; - var expected = @"using System; + var expected = @"using System; partial class Bar { partial void Foo(); @@ -509,12 +505,7 @@ partial class Bar " ; - await VerifyCustomCommitProviderAsync(text, "Foo()", expected); - } - finally - { - workspace.TryApplyChanges(workspace.CurrentSolution.WithOptions(originalOptions)); - } + await VerifyCustomCommitProviderAsync(text, "Foo()", expected); } } } diff --git a/src/EditorFeatures/CSharpTest/Formatting/CodeCleanupTests.cs b/src/EditorFeatures/CSharpTest/Formatting/CodeCleanupTests.cs index de1cc1a6af3..48ac1c90b53 100644 --- a/src/EditorFeatures/CSharpTest/Formatting/CodeCleanupTests.cs +++ b/src/EditorFeatures/CSharpTest/Formatting/CodeCleanupTests.cs @@ -257,8 +257,7 @@ protected static async Task AssertCodeCleanupResult(string expected, string code using var workspace = TestWorkspace.CreateCSharp(code, exportProvider: exportProvider); workspace.TryApplyChanges(workspace.CurrentSolution.WithOptions(workspace.Options - .WithChangedOption(GenerationOptions.PlaceSystemNamespaceFirst, LanguageNames.CSharp, systemUsingsFirst))); - workspace.TryApplyChanges(workspace.CurrentSolution.WithOptions(workspace.Options + .WithChangedOption(GenerationOptions.PlaceSystemNamespaceFirst, LanguageNames.CSharp, systemUsingsFirst) .WithChangedOption(GenerationOptions.SeparateImportDirectiveGroups, LanguageNames.CSharp, separateUsingGroups))); // register this workspace to solution crawler so that analyzer service associate itself with given workspace diff --git a/src/EditorFeatures/CSharpTest/Formatting/FormattingEngineTestBase.cs b/src/EditorFeatures/CSharpTest/Formatting/FormattingEngineTestBase.cs index 8d5a7b34ef1..9b97f3d1ca8 100644 --- a/src/EditorFeatures/CSharpTest/Formatting/FormattingEngineTestBase.cs +++ b/src/EditorFeatures/CSharpTest/Formatting/FormattingEngineTestBase.cs @@ -127,13 +127,16 @@ internal static void AssertFormat(Workspace workspace, string expected, OptionSe protected static void AssertFormatWithView(string expectedWithMarker, string codeWithMarker, params (PerLanguageOption option, bool enabled)[] options) { using var workspace = TestWorkspace.CreateCSharp(codeWithMarker); + if (options != null) { + var optionSet = workspace.Options; foreach (var option in options) { - workspace.TryApplyChanges(workspace.CurrentSolution.WithOptions(workspace.Options - .WithChangedOption(option.option, LanguageNames.CSharp, option.enabled))); + optionSet = optionSet.WithChangedOption(option.option, LanguageNames.CSharp, option.enabled); } + + workspace.TryApplyChanges(workspace.CurrentSolution.WithOptions(optionSet)); } // set up caret position diff --git a/src/EditorFeatures/CSharpTest/Workspaces/WorkspaceTests.cs b/src/EditorFeatures/CSharpTest/Workspaces/WorkspaceTests.cs index f1ddf9fc0da..831c4a7cfa9 100644 --- a/src/EditorFeatures/CSharpTest/Workspaces/WorkspaceTests.cs +++ b/src/EditorFeatures/CSharpTest/Workspaces/WorkspaceTests.cs @@ -1212,8 +1212,9 @@ public void TestSolutionWithOptions() Assert.Equal(BackgroundAnalysisScope.ActiveFile, currentOptionValue); } - [Fact, WorkItem(19284, "https://github.com/dotnet/roslyn/issues/19284")] - public void TestOptionChangedHandlerInvokedAfterCurrentSolutionChanged() + [CombinatorialData] + [Theory, WorkItem(19284, "https://github.com/dotnet/roslyn/issues/19284")] + public void TestOptionChangedHandlerInvokedAfterCurrentSolutionChanged(bool testDeprecatedOptionsSetter) { // Create workspaces with shared global options to replicate the true global options service shared between workspaces. using var primaryWorkspace = CreateWorkspace(workspaceKind: TestWorkspaceName.NameWithSharedGlobalOptions); @@ -1238,7 +1239,16 @@ public void TestOptionChangedHandlerInvokedAfterCurrentSolutionChanged() optionService.OptionChanged += OptionService_OptionChanged; // Change workspace options through primary workspace - primaryWorkspace.SetOptions(primaryWorkspace.Options.WithChangedOption(optionKey, BackgroundAnalysisScope.ActiveFile)); + if (testDeprecatedOptionsSetter) + { +#pragma warning disable CS0618 // Type or member is obsolete - this test ensures that deprecated "Workspace.set_Options" API's functionality is preserved. + primaryWorkspace.Options = primaryWorkspace.Options.WithChangedOption(optionKey, BackgroundAnalysisScope.ActiveFile); +#pragma warning restore CS0618 // Type or member is obsolete + } + else + { + primaryWorkspace.SetOptions(primaryWorkspace.Options.WithChangedOption(optionKey, BackgroundAnalysisScope.ActiveFile)); + } // Verify current solution and option change for both workspaces. VerifyCurrentSolutionAndOptionChange(primaryWorkspace, beforeSolutionForPrimaryWorkspace); diff --git a/src/EditorFeatures/Test/CodeGeneration/CodeGenerationTests.cs b/src/EditorFeatures/Test/CodeGeneration/CodeGenerationTests.cs index 58c3c4409b0..2742c4b95d0 100644 --- a/src/EditorFeatures/Test/CodeGeneration/CodeGenerationTests.cs +++ b/src/EditorFeatures/Test/CodeGeneration/CodeGenerationTests.cs @@ -357,11 +357,13 @@ public partial class CodeGenerationTests var workspace = context.Workspace; if (options != null) { + var optionSet = workspace.Options; foreach (var kvp in options) { - workspace.TryApplyChanges(workspace.CurrentSolution.WithOptions(workspace.Options - .WithChangedOption(kvp.Key, kvp.Value))); + optionSet = optionSet.WithChangedOption(kvp.Key, kvp.Value); } + + workspace.TryApplyChanges(workspace.CurrentSolution.WithOptions(optionSet)); } var typeSymbol = GetTypeSymbol(type)(context.SemanticModel); diff --git a/src/VisualStudio/CSharp/Impl/Options/AutomationObject.cs b/src/VisualStudio/CSharp/Impl/Options/AutomationObject.cs index 17b0b12dc9f..12fea470080 100644 --- a/src/VisualStudio/CSharp/Impl/Options/AutomationObject.cs +++ b/src/VisualStudio/CSharp/Impl/Options/AutomationObject.cs @@ -13,7 +13,6 @@ using Microsoft.CodeAnalysis.Editor.Shared.Options; using Microsoft.CodeAnalysis.ExtractMethod; using Microsoft.CodeAnalysis.Options; -using Microsoft.CodeAnalysis.Shared.Options; using Microsoft.CodeAnalysis.Simplification; using Microsoft.CodeAnalysis.SymbolSearch; -- GitLab