Charp73 #173

laurentkempe · 2018-08-25T14:34:50Z

No description provided.

laurentkempe · 2018-08-25T14:48:27Z

GitDiffMargin/Core/BackgroundParser.cs

-            var t = ParseComplete;
-            if (t != null)
-                t(this, e);
+            ParseComplete?.Invoke(this, e);


@sharwell Why do you assign to local before calling?

➡️ the ?. operator also assigns to a local. It prevents a NullReferenceException if the field is assigned between the null check and the invocation.

sharwell · 2018-08-25T16:06:19Z

Looks like a fun PR. I'll go through it!

sharwell

Run R# default full cleanup

This commit is more than I can deal with. Hopefully it's all correct... 😄

laurentkempe · 2018-08-25T17:03:22Z

@sharwell Nice ! But I am not finished

laurentkempe · 2018-08-27T12:23:49Z

This commit is more than I can deal with. Hopefully it's all correct...

@sharwell I will split this PR in multiple one ok ?

sharwell · 2018-08-28T12:24:23Z

My biggest recommendation is if you are hoping/planning to settle on a consistent style, use StyleCop Analyzers to enforce the desired style during the build. Otherwise it's too easy for the project to start to deviate.

sharwell

@laurentkempe In the end, the decision is up to you. I'm not a fan of the way this tool applies formatting, but it's not my project so I don't make the rules. 😄

sharwell · 2018-08-28T12:25:26Z

GitDiffMargin.Commands/CommandHandlerTextViewCreationListener.cs

            textView.Properties.AddProperty(typeof(GitDiffMarginCommandHandler), filter);
        }
    }
-}
+}


📝 I don't like how the missing newline at the end is presented by tools to look like an error.

sharwell · 2018-08-28T12:26:04Z

GitDiffMargin.Commands/CommandHandlerTextViewCreationListener.cs

        private readonly IVsEditorAdaptersFactoryService _editorAdaptersFactoryService;
+        private readonly SVsServiceProvider _serviceProvider;


❔ Why was this reordered? Previously the fields, the constructor parameters, and the assignments in the constructor all appeared in the same order. Now they are inconsistent.

sharwell · 2018-08-28T12:26:41Z

GitDiffMargin.Commands/GitDiffMarginCommandHandler`1.cs

    internal abstract class GitDiffMarginCommandHandler<T> : ShimCommandHandler<T>
        where T : EditorCommandArgs
    {
        protected GitDiffMarginCommandHandler(GitDiffMarginCommand commandId)
-            : base(new Guid(GitDiffMarginCommandHandler.GitDiffMarginCommandSet), (uint)commandId)
+            : base(new Guid(GitDiffMarginCommandHandler.GitDiffMarginCommandSet), (uint) commandId)


📝 Unexpected space after cast. If this is a style you want to use, you'll want to add it to a .editorconfig file because it's not the default behavior of Visual Studio.

sharwell · 2018-08-28T12:27:47Z

GitDiffMargin.Commands/ShimCommandHandler.cs


        public virtual CommandState GetCommandState(T args)
        {
            ThreadHelper.ThrowIfNotOnUIThread();

-            OLECMD[] command = { new OLECMD { cmdID = _commandId } };
+            OLECMD[] command = {new OLECMD {cmdID = _commandId}};


📝 Unexpected removal of spaces inside braces

sharwell · 2018-08-28T12:28:59Z

GitDiffMargin.Commands/ShimCommandHandler.cs

+
+            return (command[0].cmdf & (uint) OLECMDF.OLECMDF_ENABLED) == 0
+                ? CommandState.Unavailable
+                : CommandState.Available;


💭 This is not wrong, but I don't like how some of this method uses an if with early return, while another part uses a ternary. Given the complexity of the condition expressions, I believe the original form was preferable.

sharwell · 2018-08-28T12:42:55Z

GitDiffMargin/Core/MarginCore.cs

+            if (e.ChangedItems.Contains(DiffFormatNames.Addition)
+                || e.ChangedItems.Contains(DiffFormatNames.Modification)
+                || e.ChangedItems.Contains(DiffFormatNames.Removed))
+                UpdateBrushes();


📝 missing braces for multi-line condition. This situation is similar to the one that led to SA1519, and IMO it was a mistake to omit this from the design of that rule (see DotNetAnalyzers/StyleCopAnalyzers#1488).

sharwell · 2018-08-28T12:44:14Z

GitDiffMargin/EditorDiffMargin.cs

@@ -48,11 +45,7 @@ private void UpdateDiffDimensions(DiffViewModel diffViewModel, HunkRangeInfo hun

        private bool? UpdateNormalDiffDimensions(DiffViewModel diffViewModel, HunkRangeInfo hunkRangeInfo)
        {
-            if (hunkRangeInfo.NewHunkRange.NumberOfLines <= 0)
-            {
-                // if visible, it would have been as a deletion


❓ Where did this comment go? Looks like the tool performed an auto-destroy on this method.

sharwell · 2018-08-28T12:45:16Z

GitDiffMargin/EditorDiffMargin.cs

@@ -194,9 +165,9 @@ private void UpdateDiffDimensions(DiffViewModel diffViewModel, HunkRangeInfo hun
                    return false;
            }

-            double center = followingTop;
-            double height = TextView.LineHeight;
-            diffViewModel.Top = center - (height / 2.0);


📝 These parentheses improve readability IMO

sharwell · 2018-08-28T12:46:45Z

GitDiffMargin/Git/GitCommands.cs

@@ -47,27 +39,14 @@ public IEnumerable<HunkRangeInfo> GetGitDiffFor(ITextDocument textDocument, stri
                    yield break;

                var retrieveStatus = repo.RetrieveStatus(originalPath);
-                if (retrieveStatus == FileStatus.Nonexistent)
-                {
-                    // this occurs if a file within the repository itself (not the working copy) is opened.


📝 Comment handling in whatever tool formatted this method is a serious problem. It's severe enough that I would recommend reporting this as a bug to the tool vendor.

sharwell · 2018-08-28T12:49:32Z

GitDiffMargin/Properties/AssemblyInfo.cs

-[assembly: InternalsVisibleTo("GitDiffMargin.LegacyCommands, PublicKey=00240000048000009400000006020000002400005253413100040000010001007df3fa608a609f848a39944defc0b31e504b3e84fc5c7cd6008277f4c323cc8332ce97434c483558e43fb4d6b5c6e4c4ddb3711dabafef0e79bda1f02d49621c7bc1da4b6847707f70417455e6b76cb27c08f4d32ad29a20233124023b809d2be10d3b0a003edeee23c0d8758b384103a18c95ba323c63a451052d84dc7672d0")]
+[assembly:
+    InternalsVisibleTo(
+        "GitDiffMargin.Commands, PublicKey=00240000048000009400000006020000002400005253413100040000010001007df3fa608a609f848a39944defc0b31e504b3e84fc5c7cd6008277f4c323cc8332ce97434c483558e43fb4d6b5c6e4c4ddb3711dabafef0e79bda1f02d49621c7bc1da4b6847707f70417455e6b76cb27c08f4d32ad29a20233124023b809d2be10d3b0a003edeee23c0d8758b384103a18c95ba323c63a451052d84dc7672d0")]


📝 I understand how this happened, but at this point it seems to make more sense to just leave it on one line.

laurentkempe · 2018-08-28T16:40:05Z

In the end, the decision is up to you. I'm not a fan of the way this tool applies formatting, but it's not my project so I don't make the rules

@sharwell I have to say that I am not a big fan neither at the moment of the formatting! It might be not your project but your participate quite a lot on it, so it is always good to get your ideas/feeling... I will reconsider all of this and will first make a PR which just change the whole thing to C# 7.3! For the rest we will see

laurentkempe added 4 commits August 25, 2018 16:36

Update all projects to use C# 7.3

284493e

Run R# default full cleanup

d75fc3b

Use nameof in place of string

712fa20

Join null check with assignement

165872d

laurentkempe force-pushed the charp73 branch from 5ad37e2 to 165872d Compare August 25, 2018 14:39

Use null propagation

94ecf03

laurentkempe commented Aug 25, 2018

View reviewed changes

laurentkempe added 4 commits August 25, 2018 16:51

Inline out variable declaration

ec22983

Convert to ?: operator

1e6cea5

Convert to return statement

5980b7a

Use pattern matching

0a5b293

laurentkempe force-pushed the charp73 branch from dc466fe to 0a5b293 Compare August 25, 2018 15:01

laurentkempe added 9 commits August 25, 2018 17:02

Convert delegate to local function

f42936b

Introduce optional parameters

f160567

Invert if statement to reduce nesting

6a3891e

Merge conditional ?: expression into contional access

32aec68

Simplify code

5b15edd

Use string interpolation

b5bd9ce

Remove useless ParseResultEventArgs class

4a89715

Delete unused local variable

dfaa74e

Remove property Name which is never used

77f43c5

sharwell reviewed Aug 25, 2018

View reviewed changes

sharwell reviewed Aug 28, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Charp73 #173

Charp73 #173

laurentkempe commented Aug 25, 2018

laurentkempe Aug 25, 2018

sharwell Aug 25, 2018

sharwell commented Aug 25, 2018

sharwell left a comment

laurentkempe commented Aug 25, 2018

laurentkempe commented Aug 27, 2018

sharwell commented Aug 28, 2018

sharwell left a comment

sharwell Aug 28, 2018

sharwell Aug 28, 2018

sharwell Aug 28, 2018 •

edited

Loading

sharwell Aug 28, 2018

sharwell Aug 28, 2018

sharwell Aug 28, 2018

sharwell Aug 28, 2018

sharwell Aug 28, 2018

sharwell Aug 28, 2018

sharwell Aug 28, 2018

laurentkempe commented Aug 28, 2018

		private readonly IVsEditorAdaptersFactoryService _editorAdaptersFactoryService;
		private readonly SVsServiceProvider _serviceProvider;

Charp73 #173

Are you sure you want to change the base?

Charp73 #173

Conversation

laurentkempe commented Aug 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell commented Aug 25, 2018

sharwell left a comment

Choose a reason for hiding this comment

laurentkempe commented Aug 25, 2018

laurentkempe commented Aug 27, 2018

sharwell commented Aug 28, 2018

sharwell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurentkempe commented Aug 28, 2018

sharwell Aug 28, 2018 •

edited

Loading