GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Update rule docs by sdwheeler · Pull Request #1711 · PowerShell/PSScriptAnalyzer

Conversation

Sean Wheeler (sdwheeler) commented Aug 31, 2021 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

PR Summary

Updated the README and Rules documentation.

Corrected grammar and formatting.
Added column to table in README to indicate which rules are enabled by default.
Renamed files to match the actual rule names as used by the end-user

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Update rule docs

4a5b2d7

Sean Wheeler (sdwheeler) force-pushed the sdw-docs branch from d2d6381 to 4a5b2d7 Compare

August 31, 2021 19:41


          Revert file name changes

Steven Bucher (StevenBucher98) reviewed

Sep 8, 2021

View reviewed changes

Comment thread

RuleDocumentation/AvoidUsingDoubleQuotesForConstantString.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Steven Bucher (StevenBucher98) reviewed

Sep 8, 2021

View reviewed changes

Comment thread

RuleDocumentation/AvoidShouldContinueWithoutForce.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Steven Bucher (StevenBucher98) reviewed

Sep 8, 2021

View reviewed changes

Comment thread

RuleDocumentation/ProvideCommentHelp.md Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Feedback edits

8ad2a59

Steven Bucher (StevenBucher98) reviewed

Sep 8, 2021

View reviewed changes

Comment thread

RuleDocumentation/ReservedCmdletChar.md Resolved

Uh oh!

There was an error while loading. Please reload this page.


          feedback edit

1ae6586

Steven Bucher (StevenBucher98) approved these changes

Sep 8, 2021

View reviewed changes

Steven Bucher (StevenBucher98) left a comment

Copy link

Copy Markdown

Collaborator

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.

Approved, mostly just formatting changes but did try to read every revised wording, I was looking for grammar and just basic high level things, not really if rules/docs were "correct" because I am unsure about every rule. I learned somethings even reading through :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Sean Wheeler (sdwheeler) enabled auto-merge (squash)

September 28, 2021 13:42

Steve Lee (SteveL-MSFT) requested changes

Sep 28, 2021

View reviewed changes

Steve Lee (SteveL-MSFT) left a comment

Copy link

Copy Markdown

Member

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.

about halfway done reviewing, will take time to go through it thoroughly

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

RuleDocumentation/AvoidGlobalVars.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

RuleDocumentation/AvoidUsingConvertToSecureStringWithPlainText.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

RuleDocumentation/AvoidUsingPlainTextForPassword.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

RuleDocumentation/PossibleIncorrectComparisonWithNull.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Rob Holt (rjmholt) reviewed

Sep 28, 2021

View reviewed changes

Comment thread

RuleDocumentation/PSPossibleIncorrectComparisonWithNull.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

RuleDocumentation/PSReservedCmdletChar.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

RuleDocumentation/PSUseCompatibleCommands.md

Comment on lines +78 to +82

		The compatibility analysis compares a command used to both a target profile and a "union" profile
		(containing all commands available in any profile in the profile dir). If a command is not present
		in the union profile, it is assumed to be locally created and ignored. Otherwise, if a command is
		present in the union profile but not present in a target, it is deemed to be incompatible with that
		target.

Rob Holt (rjmholt) Aug 31, 2021

Copy link

Copy Markdown

Contributor

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.

This documentation was written with semantic line breaks in mind. While those aren't used in the docs repos, I think we wish to keep them here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

RuleDocumentation/PSUseLiteralInitializerForHashtable.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

RuleDocumentation/AvoidOverwritingBuiltInCmdlets.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

RuleDocumentation/PossibleIncorrectUsageOfAssignmentOperator.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

RuleDocumentation/UseCompatibleSyntax.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

RuleDocumentation/UsePSCredentialType.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

RuleDocumentation/UseUsingScopeModifierInNewRunspaces.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

RuleDocumentation/UseUsingScopeModifierInNewRunspaces.md

		If a ScriptBlock is intended to be run in a new RunSpace, variables inside it should use the
		`$using:` scope modifier, or be initialized within the ScriptBlock. This applies to:

		- `Invoke-Command`- Only with the ComputerName or Session parameter.

Rob Holt (rjmholt) Sep 28, 2021

Copy link

Copy Markdown

Contributor

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.

Here I'd suggest -ComputerName etc, in monospace

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Sean Wheeler (sdwheeler) Sep 29, 2021

Copy link

Copy Markdown

Contributor Author

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.

Bold is the style when parameter names are used in a paragraph. Code style is used when the parameter is used in a code context.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) reviewed

Sep 29, 2021

View reviewed changes

Comment thread

RuleDocumentation/UseBOMForUnicodeEncodedFile.md

		## Description

		For a file encoded with a format other than ASCII, ensure Byte Order Mark (BOM) is present to ensure that any application consuming this file can interpret it correctly.
		For a file encoded with a format other than ASCII, ensure Byte Order Mark (BOM) is present to ensure

Steve Lee (SteveL-MSFT) Sep 29, 2021

Copy link

Copy Markdown

Member

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.

Is this still true or do we expect UTF8 (no BOM) now?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Sean Wheeler (sdwheeler) Sep 29, 2021

Copy link

Copy Markdown

Contributor Author

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.

The rule exists. I guess the question is whether it is or should be applied?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Rob Holt (rjmholt) Sep 29, 2021

Copy link

Copy Markdown

Contributor

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.

Steve Lee (@SteveL-MSFT) the rule enforces the presence of a BOM.

A script encoded with UTF-8 (no BOM) will be misinterpreted by WinPS as CP-1252.

See https://docs.microsoft.com/powershell/scripting/dev-cross-plat/vscode/understanding-file-encoding

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Rob Holt (rjmholt) Sep 29, 2021

Copy link

Copy Markdown

Contributor

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.

(As it happens, that doc page itself has encoding issues around HTML escaping)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bergmeister reacted with laugh emoji

All reactions

😄 1 reaction

Sean Wheeler (sdwheeler) Sep 29, 2021

Copy link

Copy Markdown

Contributor Author

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.

Should we add the information about WinPS to the rule doc?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Sean Wheeler (sdwheeler) Sep 29, 2021

Copy link

Copy Markdown

Contributor Author

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.

Rob Holt (@rjmholt) FYI - just fixed the html encoding problem in the article. It will be live this afternoon.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rjmholt reacted with thumbs up emoji

All reactions

👍 1 reaction

Sean Wheeler (sdwheeler) and others added 3 commits

September 29, 2021 09:12


          Apply suggestions from code review

72494ad

Co-authored-by: Robert Holt <[email protected]>
Co-authored-by: Steve Lee <[email protected]>


          Feedback edits

0e3cbdc


          More feedback edits

a53661c

Rob Holt (rjmholt) approved these changes

Sep 29, 2021

View reviewed changes

Rob Holt (rjmholt) requested a review from Steve Lee (SteveL-MSFT)

September 29, 2021 18:27

Rob Holt (rjmholt) commented Sep 29, 2021

Copy link

Copy Markdown

Contributor

Steve Lee (@SteveL-MSFT) this is set to auto-merge, so when you refresh your review to an approval it will merge

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Steve Lee (SteveL-MSFT) approved these changes

Sep 29, 2021

View reviewed changes

Sean Wheeler (sdwheeler) merged commit 00bea7a into PowerShell:master

Sep 29, 2021

Sean Wheeler (sdwheeler) deleted the sdw-docs branch

October 4, 2021 17:20

Reviewers

Steve Lee (SteveL-MSFT)

SteveL-MSFT approved these changes

Steven Bucher (StevenBucher98)

StevenBucher98 approved these changes

+1 more reviewer

Rob Holt (rjmholt)

rjmholt approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

Sean Wheeler (sdwheeler) commented Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Steven Bucher (StevenBucher98) left a comment

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Rob Holt (rjmholt) commented Sep 29, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Sean Wheeler (sdwheeler) commented Aug 31, 2021 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.