-
Notifications
You must be signed in to change notification settings - Fork 237
Add linting to PSES, with fixes for obvious cases #1155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 11 commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
7153ea0
Use culture invariant int render
rjmholt e6330f8
Suppress unhelpful lint messages
rjmholt 3950e51
Remove useless .editorconfig line
rjmholt cb591e0
Add suppressions file to PSES project
rjmholt 2f7ab8d
Add ConfigureAwait(false) to await calls
rjmholt 9b95814
Check nulls on public APIs
rjmholt cbd6074
More ConfigureAwait(false)
rjmholt ddbf7f6
Ensure BuildInfo data is parsed properly
rjmholt 1f020af
Suggested changes
rjmholt a79e712
Fix suppression files
rjmholt cb8571f
Update src/PowerShellEditorServices/Services/PowerShellContext/PowerS…
rjmholt 5755d5e
Use Validate for null check
rjmholt 886004c
Merged fix
rjmholt 8e6ffd7
Use invariant datetime in build
rjmholt 74a00c0
Use validate
rjmholt d006692
Add rule suppressions
rjmholt 16f8e1e
Add ConfigureAwait(false) to await calls
rjmholt 3647290
Suppress non-awaited tasks
rjmholt 6b0799a
Use Array.Empty() instead of new []
rjmholt f5ab17c
Fix common tests
rjmholt 0a9ec86
Suppress cctor
rjmholt fd9ea79
Try to solve Codacy error
rjmholt File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,11 @@ | ||
using System.Globalization; | ||
|
||
namespace Microsoft.PowerShell.EditorServices.Hosting | ||
{ | ||
public static class BuildInfo | ||
{ | ||
public static readonly string BuildVersion = "<development-build>"; | ||
public static readonly string BuildOrigin = "<development>"; | ||
public static readonly System.DateTime? BuildTime = System.DateTime.Parse("2019-12-06T21:43:41"); | ||
public static readonly System.DateTime? BuildTime = System.DateTime.Parse("2019-12-06T21:43:41", CultureInfo.InvariantCulture.DateTimeFormat); | ||
} | ||
} |
This file contains hidden or 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
This file contains hidden or 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
11 changes: 11 additions & 0 deletions
11
src/PowerShellEditorServices.Hosting/GlobalSuppressions.cs
This file contains hidden or 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
// This file is used by Code Analysis to maintain SuppressMessage | ||
// attributes that are applied to this project. | ||
// Project-level suppressions either have no target or are given | ||
// a specific target and scoped to a namespace, type, member, etc. | ||
|
||
[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")] | ||
|
||
[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "CA2208:Instantiate argument exceptions correctly", Justification = "Checking user input from a configuration", Scope = "member", Target = "~M:Microsoft.PowerShell.EditorServices.Hosting.EditorServicesLoader.ValidateConfiguration")] |
This file contains hidden or 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
// This file is used by Code Analysis to maintain SuppressMessage | ||
// attributes that are applied to this project. | ||
// Project-level suppressions either have no target or are given | ||
// a specific target and scoped to a namespace, type, member, etc. | ||
|
||
[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("Reliability", "CA2000:Dispose objects before losing scope", Justification = "Disposed by created object", Scope = "member", Target = "~M:Microsoft.PowerShell.EditorServices.Hosting.EditorServicesServerFactory.Create(System.String,System.Int32,System.IObservable{System.})~Microsoft.PowerShell.EditorServices.Hosting.EditorServicesServerFactory")] | ||
[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Performance", "CA1810:Initialize reference type static fields inline", Justification = "cctor required for version-specific behavior", Scope = "member", Target = "~M:Microsoft.PowerShell.EditorServices.Services.PowerShellContextService.#cctor")] | ||
[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "PowerShellContext must catch all exceptions for robustness, logging them instead", Scope = "member", Target = "~M:Microsoft.PowerShell.EditorServices.Services.PowerShellContextService.ExecuteCommandAsync``1(System.Management.Automation.PSCommand,System.Text.StringBuilder,Microsoft.PowerShell.EditorServices.Services.PowerShellContext.ExecutionOptions)~System.Threading.Tasks.Task{System.Collections.Generic.IEnumerable{``0}}")] |
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,7 @@ namespace Microsoft.PowerShell.EditorServices.Services.PowerShellContext | |
internal class ConsoleReadLine | ||
{ | ||
#region Private Field | ||
private PowerShellContextService powerShellContext; | ||
private readonly PowerShellContextService powerShellContext; | ||
|
||
#endregion | ||
|
||
|
@@ -48,8 +48,9 @@ public async Task<SecureString> ReadSecureLineAsync(CancellationToken cancellati | |
{ | ||
SecureString secureString = new SecureString(); | ||
|
||
int initialPromptRow = await ConsoleProxy.GetCursorTopAsync(cancellationToken); | ||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
int previousInputLength = 0; | ||
|
||
Console.TreatControlCAsInput = true; | ||
|
@@ -58,7 +59,7 @@ public async Task<SecureString> ReadSecureLineAsync(CancellationToken cancellati | |
{ | ||
while (!cancellationToken.IsCancellationRequested) | ||
{ | ||
ConsoleKeyInfo keyInfo = await ReadKeyAsync(cancellationToken); | ||
ConsoleKeyInfo keyInfo = await ReadKeyAsync(cancellationToken).ConfigureAwait(false); | ||
|
||
if ((int)keyInfo.Key == 3 || | ||
keyInfo.Key == ConsoleKey.C && keyInfo.Modifiers.HasFlag(ConsoleModifiers.Control)) | ||
|
@@ -96,8 +97,8 @@ public async Task<SecureString> ReadSecureLineAsync(CancellationToken cancellati | |
} | ||
else if (previousInputLength > 0 && currentInputLength < previousInputLength) | ||
{ | ||
int row = await ConsoleProxy.GetCursorTopAsync(cancellationToken); | ||
int col = await ConsoleProxy.GetCursorLeftAsync(cancellationToken); | ||
int row = await ConsoleProxy.GetCursorTopAsync(cancellationToken).ConfigureAwait(false); | ||
int col = await ConsoleProxy.GetCursorLeftAsync(cancellationToken).ConfigureAwait(false); | ||
|
||
// Back up the cursor before clearing the character | ||
col--; | ||
|
@@ -127,14 +128,14 @@ public async Task<SecureString> ReadSecureLineAsync(CancellationToken cancellati | |
|
||
#region Private Methods | ||
|
||
private static async Task<ConsoleKeyInfo> ReadKeyAsync(CancellationToken cancellationToken) | ||
private static Task<ConsoleKeyInfo> ReadKeyAsync(CancellationToken cancellationToken) | ||
{ | ||
return await ConsoleProxy.ReadKeyAsync(intercept: true, cancellationToken); | ||
return ConsoleProxy.ReadKeyAsync(intercept: true, cancellationToken); | ||
} | ||
|
||
private async Task<string> ReadLineAsync(bool isCommandLine, CancellationToken cancellationToken) | ||
private Task<string> ReadLineAsync(bool isCommandLine, CancellationToken cancellationToken) | ||
{ | ||
return await this.powerShellContext.InvokeReadLineAsync(isCommandLine, cancellationToken); | ||
return this.powerShellContext.InvokeReadLineAsync(isCommandLine, cancellationToken); | ||
} | ||
|
||
/// <summary> | ||
|
@@ -154,6 +155,7 @@ private async Task<string> ReadLineAsync(bool isCommandLine, CancellationToken c | |
/// </returns> | ||
internal async Task<string> InvokeLegacyReadLineAsync(bool isCommandLine, CancellationToken cancellationToken) | ||
{ | ||
// TODO: Is inputBeforeCompletion used? | ||
string inputBeforeCompletion = null; | ||
string inputAfterCompletion = null; | ||
CommandCompletion currentCompletion = null; | ||
|
@@ -163,9 +165,10 @@ internal async Task<string> InvokeLegacyReadLineAsync(bool isCommandLine, Cancel | |
|
||
StringBuilder inputLine = new StringBuilder(); | ||
|
||
int initialCursorCol = await ConsoleProxy.GetCursorLeftAsync(cancellationToken); | ||
int initialCursorRow = await ConsoleProxy.GetCursorTopAsync(cancellationToken); | ||
int initialCursorCol = await ConsoleProxy.GetCursorLeftAsync(cancellationToken).ConfigureAwait(false); | ||
int initialCursorRow = await ConsoleProxy.GetCursorTopAsync(cancellationToken).ConfigureAwait(false); | ||
|
||
// TODO: Are these used? | ||
int initialWindowLeft = Console.WindowLeft; | ||
int initialWindowTop = Console.WindowTop; | ||
|
||
|
@@ -177,7 +180,7 @@ internal async Task<string> InvokeLegacyReadLineAsync(bool isCommandLine, Cancel | |
{ | ||
while (!cancellationToken.IsCancellationRequested) | ||
{ | ||
ConsoleKeyInfo keyInfo = await ReadKeyAsync(cancellationToken); | ||
ConsoleKeyInfo keyInfo = await ReadKeyAsync(cancellationToken).ConfigureAwait(false); | ||
|
||
// Do final position calculation after the key has been pressed | ||
// because the window could have been resized before then | ||
|
@@ -207,14 +210,15 @@ internal async Task<string> InvokeLegacyReadLineAsync(bool isCommandLine, Cancel | |
command.AddParameter("CursorColumn", currentCursorIndex); | ||
command.AddParameter("Options", null); | ||
|
||
var results = | ||
await this.powerShellContext.ExecuteCommandAsync<CommandCompletion>(command, false, false); | ||
var results = await this.powerShellContext | ||
.ExecuteCommandAsync<CommandCompletion>(command, sendOutputToHost: false, sendErrorToHost: false) | ||
.ConfigureAwait(false); | ||
|
||
currentCompletion = results.FirstOrDefault(); | ||
} | ||
else | ||
{ | ||
using (RunspaceHandle runspaceHandle = await this.powerShellContext.GetRunspaceHandleAsync()) | ||
using (RunspaceHandle runspaceHandle = await this.powerShellContext.GetRunspaceHandleAsync().ConfigureAwait(false)) | ||
using (PowerShell powerShell = PowerShell.Create()) | ||
{ | ||
powerShell.Runspace = runspaceHandle.Runspace; | ||
|
@@ -325,11 +329,9 @@ internal async Task<string> InvokeLegacyReadLineAsync(bool isCommandLine, Cancel | |
PSCommand command = new PSCommand(); | ||
command.AddCommand("Get-History"); | ||
|
||
currentHistory = | ||
await this.powerShellContext.ExecuteCommandAsync<PSObject>( | ||
command, | ||
false, | ||
false) as Collection<PSObject>; | ||
currentHistory = await this.powerShellContext.ExecuteCommandAsync<PSObject>(command, sendOutputToHost: false, sendErrorToHost: false) | ||
.ConfigureAwait(false) | ||
as Collection<PSObject>; | ||
|
||
if (currentHistory != null) | ||
{ | ||
|
@@ -475,7 +477,7 @@ await this.powerShellContext.ExecuteCommandAsync<PSObject>( | |
inputLine, | ||
promptStartCol, | ||
promptStartRow, | ||
keyInfo.KeyChar.ToString(), | ||
keyInfo.KeyChar.ToString(), // TODO: Determine whether this should take culture into account | ||
currentCursorIndex, | ||
finalCursorIndex: currentCursorIndex + 1); | ||
} | ||
|
@@ -489,6 +491,7 @@ await this.powerShellContext.ExecuteCommandAsync<PSObject>( | |
return null; | ||
} | ||
|
||
// TODO: Is this used? | ||
private int CalculateIndexFromCursor( | ||
int promptStartCol, | ||
int promptStartRow, | ||
|
This file contains hidden or 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could these be placed inline? The ones that target a specific member that is.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can certainly see the pros of having the attribute inline, so you get the justification like a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally agree with @SeeminglyScience about putting them inline so they're "self documenting"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went for the attribute where possible, just since it's got a field for
Justification
. Some things required a pragma though