Add suggested corrections to allow violation fixes #508

kapilmb · 2016-04-19T01:05:32Z

This change is

The class does not implement IScriptExtent interface anymore. The purpose of CorrectionExtent is to emit violation extent and the correction text but this behaviour is not consistent with the IScriptExtent interface.

Moves the module manifest testing logic to a separate method in the Helper class. This will enable reuse of the functionality by other rules.

Suggest corrections to FunctionsToExport and AliasesToExport when their values are set to '*' or '$null', e.g., FunctionsToExport = '*'. This does not support wildcard in an array, eg @('Set-Foo', 'Get-*').

Sets the path of the test helper module correctly.

raghushantha · 2016-04-19T16:39:42Z

README.md, line 197 [r1] (raw file):
Add documentation to about help topic

Comments from Reviewable

raghushantha · 2016-04-19T16:40:09Z

Reviewed 23 of 23 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

raghushantha · 2016-04-19T16:40:48Z

README.md, line 197 [r1] (raw file):
https://github.com/PowerShell/PSScriptAnalyzer/blob/development/Engine/about_PSScriptAnalyzer.help.txt

Comments from Reviewable

raghushantha · 2016-04-20T17:17:09Z

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

raghushantha · 2016-04-20T17:17:35Z

Comments from Reviewable

kapilmb · 2016-04-20T19:00:33Z

@raghushantha Thanks!

lzybkr · 2016-04-20T22:42:42Z

Review status: all files reviewed at latest revision, 13 unresolved discussions, all commit checks successful.

Engine/Helper.cs, line 238 [r2] (raw file):
Should/can this be static?

Engine/Helper.cs, line 243 [r2] (raw file):
It might make sense for ScriptAnalyzer to provider a runspace pool to avoid creating too many runspaces - it does take ~50ms to create a default runspace, so that time can add up quickly depending on how many you create.

Engine/Helper.cs, line 244 [r2] (raw file):
This can never happen, it would throw an OutOfMemoryException instead.

Engine/Helper.cs, line 257 [r2] (raw file):
Terminating errors would turn into exceptions, you shouldn't ignore them.

Engine/Helper.cs, line 277 [r2] (raw file):
Should this be static?

Engine/Generic/DiagnosticRecord.cs, line 100 [r2] (raw file):
Collections are not recommended for public properties unless they are ReadOnly.

Rules/AvoidAlias.cs, line 79 [r2] (raw file):
It would be better to use cmdAst.CommandElements[0] to get the extent of the command name, it will correctly handle PowerShell escape sequences that IndexOf wouldn't. This relies on GetCommandName returning null if CommandElements[0] is not a string constant.

Rules/AvoidAlias.cs, line 81 [r2] (raw file):
Do you not localize strings in ScriptAnalyzer? "Replace..." is a localizable string.

Rules/AvoidUsingPlainTextForPassword.cs, line 76 [r2] (raw file):
Your correction here would remove attributes, e.g.

[string] [Parameter(ValueFromPipeline)] $Password
or
[Parameter(ValueFromPipeline)] $Password
or
[Parameter(ValueFromPipeline)] [string[]] $Password

It would also remove comments:

[string] <#whatever#> $Password

I think it's smart to use the parameter extent to show what gets "fixed", but more care is needed to replace only what you mean to.

Rules/MissingModuleManifestField.cs, line 47 [r2] (raw file):
IsMissingManifestMember instead of IsMissingMember?

Rules/MissingModuleManifestField.cs, line 51 [r2] (raw file):
Will you add a correction for the missing manifest member? That will be very helpful and ensure folks get it right.

Rules/UseToExportFieldsInManifest.cs, line 36 [r2] (raw file):
It's not (currently) important to specify VariablesToExport, so you could exclude this.

Rules/UseToExportFieldsInManifest.cs, line 100 [r2] (raw file):
You may want to wrap this and indent, for some modules this will look terrible (e.g. Azure exports 700+ commands).

Comments from Reviewable

lzybkr · 2016-04-20T23:31:15Z

Review status: all files reviewed at latest revision, 14 unresolved discussions.

Tests/Rules/UseToExportFieldsInManifest.tests.ps1, line 89 [r2] (raw file):
You should some additional tests,:

where the entry was missing but stuff is exported
where the entry is present, uses a wildcard, and nothing is exported
where the entry is missing, nothing is exported, the suggestion should have an empty array like FunctionsToExport = @()

Comments from Reviewable

This implementation correctly handles PowerShell escape sequences that IndexOf cannot.

Handles the following cases * [string] [Parameter(ValueFromPipeline)] $Password * [Parameter(ValueFromPipeline)] $Password * [Parameter(ValueFromPipeline)] [string[]] $Password * [string] <#whatever#> $Password

* Doesn't check for VariablesToExport * Adds ability to wrap around correction string

Adds exception handling for a particular case in GetModuleManifest method

kapilmb · 2016-04-23T16:32:57Z

Review status: all files reviewed at latest revision, 14 unresolved discussions.

Engine/Helper.cs, line 238 [r2] (raw file):
Yes, this can be static in its current form. If we were to use runspace pool going forward that would change the implementation of this method. Will update its signature during that iteration.

Engine/Helper.cs, line 243 [r2] (raw file):
Will add it in a separate PR.

Rules/MissingModuleManifestField.cs, line 51 [r2] (raw file):
Yes, that is in the plan. Will send a separate PR for that.

Tests/Rules/UseToExportFieldsInManifest.tests.ps1, line 89 [r2] (raw file):
These features are not implemented in the rule itself as of now. The rule only checks for the presence of a wildcard character or $null. Will create a separate PR for this issue.

Comments from Reviewable

Kapil Borle added 16 commits March 23, 2016 11:59

Add SuggestdCorrection property to DiagnosticRecord

d8a2a67

Set SuggestedCorrection property in AvoidAlias rule

c8fe6d6

Set SuggestedCorrection property in MisleadingBacktick rule

a18446e

Add an extent type to encapsulate correction information

50706a1

Modify CorrectionExtent class

e997906

The class does not implement IScriptExtent interface anymore. The purpose of CorrectionExtent is to emit violation extent and the correction text but this behaviour is not consistent with the IScriptExtent interface.

Modify correction extent of avoid using alias

d811566

Modify correction extent of misleading backticks rule

eb05a8e

Add a file that contains helper functions for tests

b0a7c57

Add correction extent to AvoidUsingPlainTextForPassword rule

8a02fa7

Refactor some tests that check correction extent

e65b9a9

Fix a bug in a test helper function

567b1ae

Refactor MissingModuleManifest Rule

cc68a2e

Moves the module manifest testing logic to a separate method in the Helper class. This will enable reuse of the functionality by other rules.

Add correction extent to UseToExportFieldsInManifest rule

afccf35

Suggest corrections to FunctionsToExport and AliasesToExport when their values are set to '*' or '$null', e.g., FunctionsToExport = '*'. This does not support wildcard in an array, eg @('Set-Foo', 'Get-*').

Add Description property to CorrectionExtent class

10bcbdc

Add CorrectionExtent description to some rules

8072873

Add violation correction to readme

7de186a

msftclas added the cla-already-signed label Apr 19, 2016

kapilmb added the in progress label Apr 19, 2016

Fix failing tests

804ead7

Sets the path of the test helper module correctly.

Add violation correction section to about_PSScriptScriptAnalyzer

ba7463b

Kapil Borle added 2 commits April 22, 2016 21:34

Add localized strings for correction description

40d0708

Change SuggestedCorrections type to IEnumerable

c62cdca

Kapil Borle added 5 commits April 22, 2016 21:41

Fix AvoidAlias correction extent

d85e978

This implementation correctly handles PowerShell escape sequences that IndexOf cannot.

Fix AvoidUsingPlainTextForPassword correction extent

13c5c3f

Handles the following cases * [string] [Parameter(ValueFromPipeline)] $Password * [Parameter(ValueFromPipeline)] $Password * [Parameter(ValueFromPipeline)] [string[]] $Password * [string] <#whatever#> $Password

Rename and make static a method in Helper

c4065e7

Modify UseToExportFieldInManifest rule

6fcf059

* Doesn't check for VariablesToExport * Adds ability to wrap around correction string

Modify a helper method

5e29444

Adds exception handling for a particular case in GetModuleManifest method

Resolve merge conflict

091c71d

kapilmb mentioned this pull request Apr 23, 2016

Suggested corrections for violations #502

Closed

daviwil mentioned this pull request Apr 25, 2016

Add support for VS Code's "code action" feature PowerShell/PowerShellEditorServices#197

Closed

kapilmb merged commit 091c71d into development Apr 29, 2016

kapilmb removed the in progress label Apr 29, 2016

kapilmb mentioned this pull request May 3, 2016

Which rules should provide suggested corrections? #516

Open

kapilmb deleted the AddSuggestedCorrections branch May 3, 2016 23:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add suggested corrections to allow violation fixes #508

Add suggested corrections to allow violation fixes #508

Uh oh!

kapilmb commented Apr 19, 2016

Uh oh!

raghushantha commented Apr 19, 2016

Uh oh!

raghushantha commented Apr 19, 2016

Uh oh!

raghushantha commented Apr 19, 2016

Uh oh!

raghushantha commented Apr 20, 2016

Uh oh!

raghushantha commented Apr 20, 2016

Uh oh!

kapilmb commented Apr 20, 2016

Uh oh!

lzybkr commented Apr 20, 2016

Uh oh!

lzybkr commented Apr 20, 2016

Uh oh!

kapilmb commented Apr 23, 2016

Uh oh!

Uh oh!

Add suggested corrections to allow violation fixes #508

Add suggested corrections to allow violation fixes #508

Uh oh!

Conversation

kapilmb commented Apr 19, 2016

Uh oh!

raghushantha commented Apr 19, 2016

Uh oh!

raghushantha commented Apr 19, 2016

Uh oh!

raghushantha commented Apr 19, 2016

Uh oh!

raghushantha commented Apr 20, 2016

Uh oh!

raghushantha commented Apr 20, 2016

Uh oh!

kapilmb commented Apr 20, 2016

Uh oh!

lzybkr commented Apr 20, 2016

Uh oh!

lzybkr commented Apr 20, 2016

Uh oh!

kapilmb commented Apr 23, 2016

Uh oh!

Uh oh!