Skip to content

Do not trigger UseShouldProcessForStateChangingFunctions rule for workflows #923

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

Conversation

bergmeister
Copy link
Collaborator

PR Summary

Fixes #922

Workflows do not allow SupportsShouldProcess (the parser does not even allow it), therefore do not trigger UseShouldProcessForStateChangingFunctions
As part of this I also update the copyright header and tidied up unused using statements.

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets. Please mark anything not applicable to this PR NA.

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • User facing documentation needed
  • Change is not breaking
  • Make sure you've added a new test if existing tests do not effectively test the code changed
  • This PR is ready to merge and is not work in progress
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

@bergmeister bergmeister self-assigned this Mar 5, 2018
@bergmeister bergmeister changed the title Workflows do not allow SupportsShouldProcess --> do not trigger UseShouldProcessForStateChangingFunctions rule Do not trigger UseShouldProcessForStateChangingFunctions rule for workflows Mar 5, 2018
@bergmeister bergmeister requested a review from JamesWTruher March 5, 2018 19:32
@@ -42,5 +42,11 @@ Function New-{0} () {{ }}
It "returns no violations" {
$noViolations.Count | Should -Be 0
}

It "Workflows should not trigger a warning because they do not allow SupportsShouldProcess" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to protect this from execution on core, where workflow is not supported. Add '-skip:$IsCore' I think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS> Invoke-ScriptAnalyzer -ScriptDefinition "workflow foo { 'zap' }"                                                                        
Invoke-ScriptAnalyzer : Parse error in script definition:  Workflow is not supported in PowerShell Core at line 1 column 1.
At line:1 char:1
+ Invoke-ScriptAnalyzer -ScriptDefinition "workflow foo { 'zap' }"
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : ParserError: (WorkflowNotSupportedInPowerShellCore:String) [Invoke-ScriptAnalyzer], ParseException
+ FullyQualifiedErrorId : Parse error in script definition:  Workflow is not supported in PowerShell Core at line 1 column 1.,Microsoft.Windows.PowerShell.ScriptAnalyzer.Commands.InvokeScriptAnalyzerCommand

Copy link
Collaborator Author

@bergmeister bergmeister Mar 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks, I should've run the test suite in PSCore as well.

Copy link
Contributor

@JamesWTruher JamesWTruher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@JamesWTruher JamesWTruher merged commit f002c1c into PowerShell:development Mar 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Workflows should not trigger UseShouldProcessForStateChangingFunctions
2 participants