Code clean up by JieCarolHu · Pull Request #26802 · dotnet/roslyn

JieCarolHu · 2018-05-11T18:36:00Z

Code cleanup with Format document button (Ctrl+k, d)

Customer scenario

When user ctrl+k, d on a C# document, it brings up the code cleanup info bar to ask user to configure the code cleanup options. Once user configures the options, next time they click ctrl+k, d, it will clean up using the enabled fixers and format the document.

Bugs this fixes

NA

Workarounds, if any

NA

Risk

Low, it only affects the scenario of ctrl+k, d.

Performance impact

Low, it only affects the scenario of ctrl+k, d. It adds some time for the ctrl+k,d if user configured a lot of fixers

Is this a regression from a previous update?

NA

Root cause analysis

NA

How was the bug found?

NA

Test documentation updated?

No

1. put open file editorconfig change tracking back 2. handle open file leaking when solution close with files opened case 3. don't share locks between editorconfig events and roslyn events

add editorconfig related changes

…than fix everything in the document. also not apply changes to buffer until everything is done rather than doing it per fix.

changed code fix part to use "fix all" and only fixes P1 list rather …

add expression body as one of fix to offer

…ver configured the code cleanup

CyrusNajmabadi · 2018-05-15T23:01:25Z

    <Reference Include="System.Core" />
+    <PackageReference Include="Microsoft.VisualStudio.Shell.15.0" Version="15.0.26730-alpha" />
+    <PackageReference Include="Microsoft.VisualStudio.Shell.Framework" Version="15.0.26730-alpha" />
+    <PackageReference Include="Microsoft.VisualStudio.TextManager.Interop" Version="7.10.6070" />


@jasonmalinowski Is it ok to reference these here from this package? Will this be a concern at all for vs4mac?

Correct, that can't go there. But this is PR for Personal Review, so at this point @JieCarolHu is still getting things working.

CyrusNajmabadi · 2018-05-15T23:01:52Z

+    <value>Code cleanup is not configured</value>
+  </data>
+  <data name="Config_it_now" xml:space="preserve">
+    <value>Config it now</value>


This isn't a good phrase :) Perhaps "Configure it now" instead?

CyrusNajmabadi · 2018-05-15T23:02:24Z

+    <value>Config it now</value>
+  </data>
+  <data name="Donot_show_this_again" xml:space="preserve">
+    <value>Don't show this again</value>


name does not match actual value. Also, in general, abbreviations are discouraged from localized messages. So this should be "Do not show this message again".

CyrusNajmabadi · 2018-05-15T23:02:38Z

+
+namespace Microsoft.CodeAnalysis.Editor.Implementation.Formatting
+{
+    [Export]


why the direct export?

CyrusNajmabadi · 2018-05-15T23:02:52Z

+using Microsoft.CodeAnalysis.Text;
+using Roslyn.Utilities;
+
+namespace Microsoft.CodeAnalysis.Editor.Implementation.Formatting


Why is the CodeCleanupService in the Formatting namespace?

CyrusNajmabadi · 2018-05-15T23:03:29Z

+    internal class CodeCleanupService : ICodeCleanupService
+    {
+        private static Lazy<IDictionary<PerLanguageOption<bool>, string[]>> _dictionary
+            = new Lazy<IDictionary<PerLanguageOption<bool>, string[]>>(GetCodeCleanupOptionMapping);


this needs a comment explaining what this is for. it seems very weird htat an option would map statically to any data, given that an option generally represents mutable data.

CyrusNajmabadi · 2018-05-15T23:03:44Z

Oh, this is for personal review. will wait until this is ready for comments.

heejaechang · 2018-05-18T01:51:55Z

+
+namespace Microsoft.CodeAnalysis.Editor.Implementation.Formatting
+{
+    public interface ICodeStyleConfigureService : IWorkspaceService


you probably not want this to be public type?

heejaechang · 2018-05-18T01:52:18Z

+
+namespace Microsoft.CodeAnalysis.Editor.Implementation.Formatting
+{
+    interface ICodeCleanupService


heejaechang · 2018-06-04T21:12:51Z

+            return document;
+        }
+
+        private List<string> GetEnabledDiagnosticIds(DocumentOptionSet docOptions)


IEnumerable < string >

heejaechang · 2018-06-04T21:13:06Z

+        {
+            var diagnosticIds = new List<string>();
+
+            foreach (var codeCleanupOptions in _optionDiagnosticsMappings.Keys)


(var kv in _optionDia....)

does tuple deconstruct works with keyvaluepair?

heejaechang · 2018-06-04T21:13:17Z

+
+            foreach (var codeCleanupOptions in _optionDiagnosticsMappings.Keys)
+            {
+                if (docOptions.GetOption(codeCleanupOptions))


GetOptions(kv.key)

heejaechang · 2018-06-04T21:13:25Z

+            {
+                if (docOptions.GetOption(codeCleanupOptions))
+                {
+                    diagnosticIds.AddRange(_optionDiagnosticsMappings[codeCleanupOptions]);


foreach(var id in kv.Value) yield id

by the way, is ordering matter? dictionary doesn't maintain ordering of its entries? you might want to just use ImmutableArray((option option, Immutablearray ids)) rather than dictionary since it looks like you won't actually need indexer access for your dictionary.

heejaechang · 2018-06-04T21:14:20Z

+
+        private static ImmutableDictionary<PerLanguageOption<bool>, ImmutableArray<string>> GetCodeCleanupOptionMapping()
+        {
+            var dictionary = new Dictionary<PerLanguageOption<bool>, ImmutableArray<string>>()


you can use ImmutableDictionaryBuilder rather than use Dictionary and do ToImmutableDictionary? is it to use built in dictionary object intializer?

yeah, i don't find a way to use dictionary initializer for Immutable dictionary

heejaechang · 2018-06-04T21:24:09Z

+            <GroupBox x:Name="FormatDocumentSettingsGroupBox" x:Uid="FormatDocumentSettingsGroupBox" >
+                <StackPanel x:Uid="StackPanel_2">
+                    <CheckBox x:Name="AreCodeCleanupRulesConfiguredCheckBox" x:Uid="AreCodeCleanupRulesConfiguredCheckBox" Visibility="Hidden" Height="0"/>
+                    <CheckBox x:Name="AllCSharpFormattingRulesCheckBox" x:Uid="AllCSharpFormattingRulesCheckBox" IsEnabled="False" IsChecked="True"/>


I think you don't need this option? user can't change it anyway. and hard to distinguish it with outer clean up since those will call format for their own purpose. it will be impossible for users to know why format is called anyway by whom.

This checkbox is called out (always checked, not allow to change) in @kuhlenh 's design note.

heejaechang · 2018-06-04T21:25:42Z

            </GroupBox>
+            <GroupBox x:Name="FormatDocumentSettingsGroupBox" x:Uid="FormatDocumentSettingsGroupBox" >
+                <StackPanel x:Uid="StackPanel_2">
+                    <CheckBox x:Name="AreCodeCleanupRulesConfiguredCheckBox" x:Uid="AreCodeCleanupRulesConfiguredCheckBox" Visibility="Hidden" Height="0"/>


is there reason this hidden checkbox is here?

heejaechang · 2018-06-04T21:27:10Z

+            FixLanguageFeaturesCheckBox.Content = CSharpVSResources.Fix_language_features;
+
+            // IsCodeCleanupConfiguredCheckBox is hidden all the time, and it tracks if the user ever configured the code cleanup
+            BindToOption(AreCodeCleanupRulesConfiguredCheckBox, CodeCleanupOptions.AreCodeCleanupRulesConfigured, LanguageNames.CSharp);


I think you don't need to do this but use the Option straight when needed.

heejaechang · 2018-06-04T21:27:46Z

+        internal void SetCodeCleanupAsConfigured()
+        {
+            // AreCodeCleanupConfiguredCheckBox is hidden all the time, and it tracks if the user ever configured the code cleanup
+            if (!AreCodeCleanupRulesConfiguredCheckBox.IsChecked.HasValue || AreCodeCleanupRulesConfiguredCheckBox.IsChecked.Value == false)


you can use "CodeCleanupOptions.AreCodeCleanupRulesConfigured" directly?

so, if user click "cancel" then we repeatedly show the info bar to users? we might want to split infobar display checking and whether user opt-in to use code clean to 2 different options.

heejaechang · 2018-06-04T23:11:51Z

Can you use what this does? http://source.roslyn.io/#Microsoft.VisualStudio.LanguageServices.Implementation/Options/OptionBinding.cs,12

JieCarolHu · 2018-06-05T20:36:58Z

retest windows_release_vs-integration_prtest please

JieCarolHu · 2018-06-05T20:37:16Z

retest windows_debug_spanish_unit32_prtest please

mavasani · 2018-06-05T21:35:35Z

+            return GetFixesAsync(document, range, includeSuppressionFixes, diagnosticId: null, cancellationToken);
+        }
+
+        private async Task<ImmutableArray<CodeFixCollection>> GetFixesAsync(Document document, TextSpan range, bool includeSuppressionFixes, string diagnosticId, CancellationToken cancellationToken)


diagnosticId [](start = 148, length = 12)

Nit: Rename to diagnosticIdOpt

…his user.

mavasani · 2018-06-05T21:45:58Z

        /// this can be expensive since it is force analyzing diagnostics if it doesn't have up-to-date one yet.
        /// </summary>
-        Task<IEnumerable<DiagnosticData>> GetDiagnosticsForSpanAsync(Document document, TextSpan range, bool includeSuppressedDiagnostics = false, CancellationToken cancellationToken = default);
+        Task<IEnumerable<DiagnosticData>> GetDiagnosticsForSpanAsync(Document document, TextSpan range, bool includeSuppressedDiagnostics = false, string diagnosticId = null, CancellationToken cancellationToken = default);


diagnosticId [](start = 154, length = 12)

Rename to diagnosticIdOpt and also add comment explaining the semantics for null and non-null value.

JieCarolHu · 2018-06-05T22:39:55Z

created #27497 to add more tests

mavasani · 2018-06-05T22:46:02Z

+namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Formatting
+{
+    [UseExportProvider]
+    public class CodeCleanupTests


CodeCleanupTests [](start = 17, length = 16)

Synced up with @carhu@microsoft.com offline and she plans to extend the tests, but probably not as part of this PR due to time constraints. I think we should at least have the following tests:

Simple unit tests, one per option, that verify code cleanup works fine with only single option enabled.

Unit tests with multiple options enabled, but the reported diagnostics are not conflicting, hence can all be fixed.

Unit tests with multiple options enabled, where reported diagnostics have conflicting spans and hence not all can be fixed. Is this possible for our currently chosen diagnostics/fixers?

Verify options persistence after changing the workspace option.

thanks, copied into #27497

mavasani · 2018-06-05T22:57:52Z

+            nameof(CodeCleanupOptions), nameof(AreCodeCleanupRulesConfigured), defaultValue: false,
+            storageLocations: new RoamingProfileStorageLocation("TextEditor.%LANGUAGE%.Are Code Cleanup Rules Configured"));
+
+        public static readonly PerLanguageOption<bool> NeverShowCodeCleanupInfoBarAgain = new PerLanguageOption<bool>(


NeverShowCodeCleanupInfoBarAgain [](start = 55, length = 32)

Question: Which options go into AutomationObject and which don't? @heejaechang@outlook.com probably knows the answer..

tracked in #27511

mavasani

JieCarolHu · 2018-06-06T04:10:45Z

@jinujoseph could you approve this?

jinujoseph · 2018-06-06T05:28:35Z

Approved to merge for 15.8.Preview3

jinujoseph · 2018-06-06T05:28:48Z

test windows_release_vs-integration_prtest

JieCarolHu and others added 15 commits April 23, 2018 14:40

remove usings when format document

3840a60

sort usings when format document

064a8e2

add code fix

840cad6

hack fix for the issue with apply multiple changes.

612d4a1

refactoring

2c6dd7f

3 changes

7e0fd53

1. put open file editorconfig change tracking back 2. handle open file leaking when solution close with files opened case 3. don't share locks between editorconfig events and roslyn events

Merge pull request #1 from heejaechang/editorconfig3

3981411

add editorconfig related changes

skip the 'Remove unused variable' when format document

595c157

changed code fix part to use "fix all" and only fixes P1 list rather …

82171f7

…than fix everything in the document. also not apply changes to buffer until everything is done rather than doing it per fix.

made undo experience better

7fb7b1a

Merge pull request #2 from heejaechang/codeFix

fb866f5

changed code fix part to use "fix all" and only fixes P1 list rather …

add expression body as one of fix to offer

d67d737

Merge pull request #3 from heejaechang/addExpressionBody

2f8984f

add expression body as one of fix to offer

merge code clean up from preivew1

27c738d

add code fix options in format document settings

6dbc430

JieCarolHu added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label May 11, 2018

JieCarolHu requested a review from a team as a code owner May 11, 2018 18:36

add hidden box IsCodeCleanupConfiguredCheckBox to track if the user e…

bd795d4

…ver configured the code cleanup

CyrusNajmabadi reviewed May 15, 2018

View reviewed changes

JieCarolHu added 3 commits May 15, 2018 16:20

clean up

d8e1eb2

fix some tests

358642f

disable a test temporarily

a1211e9

heejaechang reviewed May 18, 2018

View reviewed changes

heejaechang reviewed Jun 4, 2018

View reviewed changes

PR comments

c26eef1

mavasani reviewed Jun 5, 2018

View reviewed changes

Never Show this again will never show the code cleanup info bar for t…

c3f18ed

…his user.

mavasani reviewed Jun 5, 2018

View reviewed changes

rename

de159f8

mavasani reviewed Jun 5, 2018

View reviewed changes

mavasani approved these changes Jun 5, 2018

View reviewed changes

heejaechang approved these changes Jun 6, 2018

View reviewed changes

use ImmutableArray

9fa7d76

jinujoseph added the Approved to merge label Jun 6, 2018

JieCarolHu merged commit d3a28d4 into dotnet:master Jun 6, 2018

JieCarolHu deleted the codeCleanUp_master branch June 13, 2018 02:18

JieCarolHu restored the codeCleanUp_master branch June 28, 2018 00:56

Conversation

JieCarolHu commented May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented May 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heejaechang May 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heejaechang Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heejaechang Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heejaechang Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

JieCarolHu commented May 11, 2018 •

edited

Loading

heejaechang May 18, 2018 •

edited

Loading

heejaechang Jun 4, 2018 •

edited

Loading

heejaechang Jun 4, 2018 •

edited

Loading

heejaechang Jun 4, 2018 •

edited

Loading