Add linting to PSES, with fixes for obvious cases #1155

rjmholt · 2020-01-13T20:04:25Z

Adds .ConfigureAwait(false) to all await calls (and turns some into Task<T> pass throughs)
Fixes some clear cases of string rendering with culture variance
Add suppressions for things like localisation
Add null checks to public APIs
Make fields readonly where VS suggests
Various other syntactic sugar improvements

src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs

…hellContextService.cs

TylerLeonhardt

Looking good!

src/PowerShellEditorServices/Services/PowerShellContext/ExtensionService.cs

src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs

src/PowerShellEditorServices/Services/PowerShellContext/Utilities/CommandHelpers.cs

SeeminglyScience

LGTM! One optional

SeeminglyScience · 2020-01-14T00:09:04Z

src/PowerShellEditorServices.Hosting/GlobalSuppressions.cs

+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Performance", "CA1819:Properties should not return arrays", Justification = "Cmdlet parameters can be arrays", Scope = "module")]
+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Globalization", "CA1303:Do not pass literals as localized parameters", Justification = "PSES is not localized", Scope = "module")]
+
+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "PSCmdlet.ThrowTerminatingError() is used instead", Scope = "member", Target = "~M:Microsoft.PowerShell.EditorServices.Commands.StartEditorServicesCommand.EndProcessing")]


Could these be placed inline? The ones that target a specific member that is.

They can be. I've never liked those FxCop attributes that occur all over the code (especially when codebases stop using FxCop like in PowerShell), but will leave it up to others

I can certainly see the pros of having the attribute inline, so you get the justification like a comment

I personally agree with @SeeminglyScience about putting them inline so they're "self documenting"

Yeah it's far from perfect. I hate seeing them too, but it makes it confusing why one thing warns and another doesn't. Most folks aren't going to know to look in that file.

I usually go with #pragma warning disable CA1031 and #pragma warning restore CA1031 since it's slightly less aggressive, but then you lose a lot of information. Probably not suitable.

Went for the attribute where possible, just since it's got a field for Justification. Some things required a pragma though

TylerLeonhardt · 2020-01-14T00:28:57Z

src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs

@@ -1569,7 +1581,7 @@ public async Task SetWorkingDirectoryAsync(string path)
        /// <param name="isPathAlreadyEscaped">Specify false to have the path escaped, otherwise specify true if the path has already been escaped.</param>
        public async Task SetWorkingDirectoryAsync(string path, bool isPathAlreadyEscaped)
        {
-            this.InitialWorkingDirectory = path;
+            this.InitialWorkingDirectory = path ?? throw new ArgumentNullException(nameof(path));


Use the Validate.IsNotNull?

Missed this initially, but how do we feel about these methods?

They kind of mess with the call stack a little bit. It's not a huge deal, but it does make debugging slightly more tedious since you need to pick the right frame. And it makes Exception.TargetSite kinda useless.

Obviously I'm not expecting anything in this merged PR to change, just bringing it up in case any of y'all are also leaning that way. WTB macros 🙁

The Util method or the SetWorkingDirectory?

The Util methods for argument validation

Yeah they do seem very unnecessary and even have drawbacks as you've pointed out.

I'd be fine with them being removed but also not a huge problem atm

TylerLeonhardt

LGTM besides the above comments

TylerLeonhardt · 2020-01-14T04:51:56Z

Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 88
- Added 2
           

Clones added
============
- src/PowerShellEditorServices/Services/TextDocument/Handlers/CompletionHandler.cs  1
- src/PowerShellEditorServices/Services/TextDocument/Handlers/DocumentHighlightHandler.cs  3
- src/PowerShellEditorServices/Services/TextDocument/Handlers/DefinitionHandler.cs  1
- src/PowerShellEditorServices/Services/TextDocument/Handlers/DocumentSymbolHandler.cs  1

See the complete overview on Codacy

TylerLeonhardt · 2020-01-14T04:51:59Z

src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs

@@ -1143,12 +1164,12 @@ public async Task LoadHostProfilesAsync()
                {
                    PSCommand command = new PSCommand();
                    command.AddCommand(profilePath, false);
-                    await this.ExecuteCommandAsync<object>(command, true, true);
+                    await this.ExecuteCommandAsync<object>(command, sendOutputToHost: true, sendErrorToHost: true).ConfigureAwait(false);


Issue found: Remove this default value assigned to parameter 'sendErrorToHost'.

TylerLeonhardt · 2020-01-14T04:52:00Z

src/PowerShellEditorServices/Services/PowerShellContext/Console/ConsoleReadLine.cs

-            int initialPromptCol = await ConsoleProxy.GetCursorLeftAsync(cancellationToken);
+            // TODO: Are these values used?
+            int initialPromptRow = await ConsoleProxy.GetCursorTopAsync(cancellationToken).ConfigureAwait(false);
+            int initialPromptCol = await ConsoleProxy.GetCursorLeftAsync(cancellationToken).ConfigureAwait(false);


Issue found: Remove the unused local variable 'initialPromptCol'.

TylerLeonhardt

LGTM

rjmholt added 9 commits January 13, 2020 10:11

Use culture invariant int render

7153ea0

Suppress unhelpful lint messages

e6330f8

Remove useless .editorconfig line

3950e51

Add suppressions file to PSES project

cb591e0

Add ConfigureAwait(false) to await calls

2f7ab8d

Check nulls on public APIs

9b95814

More ConfigureAwait(false)

cbd6074

Ensure BuildInfo data is parsed properly

ddbf7f6

Suggested changes

1f020af

rjmholt requested review from TylerLeonhardt and SeeminglyScience January 13, 2020 20:04

Fix suppression files

a79e712

rjmholt commented Jan 13, 2020

View reviewed changes

src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs Outdated Show resolved Hide resolved

Update src/PowerShellEditorServices/Services/PowerShellContext/PowerS…

cb8571f

…hellContextService.cs

TylerLeonhardt reviewed Jan 13, 2020

View reviewed changes

rjmholt added 2 commits January 13, 2020 16:03

Use Validate for null check

5755d5e

Merged fix

886004c

PowerShell deleted a comment from rjmholt Jan 14, 2020

Use invariant datetime in build

8e6ffd7

SeeminglyScience approved these changes Jan 14, 2020

View reviewed changes

TylerLeonhardt reviewed Jan 14, 2020

View reviewed changes

TylerLeonhardt approved these changes Jan 14, 2020

View reviewed changes

rjmholt added 8 commits January 13, 2020 16:37

Use validate

74a00c0

Add rule suppressions

d006692

Add ConfigureAwait(false) to await calls

16f8e1e

Suppress non-awaited tasks

3647290

Use Array.Empty() instead of new []

6b0799a

Fix common tests

f5ab17c

Suppress cctor

0a9ec86

Try to solve Codacy error

fd9ea79

TylerLeonhardt reviewed Jan 14, 2020

View reviewed changes

TylerLeonhardt approved these changes Jan 14, 2020

View reviewed changes

rjmholt merged commit 81ba0c0 into PowerShell:master Jan 14, 2020

Add linting to PSES, with fixes for obvious cases #1155

Add linting to PSES, with fixes for obvious cases #1155

Uh oh!

Conversation

rjmholt commented Jan 13, 2020

Uh oh!

Uh oh!

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SeeminglyScience left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjmholt Jan 14, 2020 • 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!

TylerLeonhardt Jan 14, 2020 • 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!

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Uh oh!

TylerLeonhardt commented Jan 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rjmholt Jan 14, 2020 •

edited

Loading

TylerLeonhardt Jan 14, 2020 •

edited

Loading