Skip to content

Fix errors in ShouldProcess rule document - #1766

Merged
James Truher (JamesWTruher) merged 7 commits into
PowerShell:masterfrom
masaru-iritani:patch-1
Mar 30, 2022
Merged

Fix errors in ShouldProcess rule document#1766
James Truher (JamesWTruher) merged 7 commits into
PowerShell:masterfrom
masaru-iritani:patch-1

Conversation

@masaru-iritani

Copy link
Copy Markdown
Contributor

PR Summary

  • Fix wrong variable names in examples.
  • Remove Write-Host from the correct example. It is duplicated with outputs of -WhatIf or -Confirm. It also violates "Avoid Using Write-Host" rule.
  • Correct the severity of ShouldProcess rule as mentioned in ShouldProcess.md. Get-ScriptAnalyzerRule -Name PSShouldProcess | % Severity also returns "Warning" with ScriptAnalyzer 1.20.0

PR Checklist

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Change is not breaking
  • Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
  • Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
  • 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.
  • Update examples can run successfully on PowerShell 7.2.1.
  • Script Analyzer 1.20.0 detects a ShouldProcess violation in the updated wrong example. For some reason, Script Analyzer 1.20.0 doesn't detect any ShouldProcess violation in the updated wrong example.
  • Script Analyzer 1.20.0 detects no error in the updated correct example.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Awesome, thank you for the effort and very detailed description 🥳
Sean Wheeler (@sdwheeler) I am happy from a technical perspective, do you want to review the docs change as well before merging?

Comment thread docs/Rules/README.md
Comment thread docs/Rules/ShouldProcess.md Outdated

@sdwheeler Sean Wheeler (sdwheeler) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

One minor change to link to about_* topics.

@masaru-iritani

Copy link
Copy Markdown
Contributor Author

One minor change to link to about_* topics.

Updated as suggested. Would you review it again and resolve the request? I cannot find any way to resolve this thread by myself.

@bergmeister

Copy link
Copy Markdown
Collaborator

Sean Wheeler (@sdwheeler) Can you re-review please? You can contact James Truher (@JamesWTruher) to get it merged as the left-over check doesn't seem to go away when pulling in master.

Comment thread docs/Rules/ShouldProcess.md Outdated
@bergmeister

Copy link
Copy Markdown
Collaborator

Thanks Sean Wheeler (@sdwheeler), can you resolve the merge conflict please and then we are good to merge 💪🏻

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.

4 participants