Conversation
StevenBucher98
left a comment
StevenBucher98
left a comment
There was a problem hiding this comment.
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 :)
SteveL-MSFT
left a comment
SteveL-MSFT
left a comment
There was a problem hiding this comment.
about halfway done reviewing, will take time to go through it thoroughly
RuleDocumentation/AvoidUsingConvertToSecureStringWithPlainText.md
Outdated
Show resolved
Hide resolved
| 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. |
There was a problem hiding this comment.
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.
RuleDocumentation/PossibleIncorrectUsageOfAssignmentOperator.md
Outdated
Show resolved
Hide resolved
| 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. |
There was a problem hiding this comment.
Here I'd suggest -ComputerName etc, in monospace
There was a problem hiding this comment.
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.
| ## 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 |
There was a problem hiding this comment.
Is this still true or do we expect UTF8 (no BOM) now?
There was a problem hiding this comment.
The rule exists. I guess the question is whether it is or should be applied?
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
(As it happens, that doc page itself has encoding issues around HTML escaping)
There was a problem hiding this comment.
Should we add the information about WinPS to the rule doc?
There was a problem hiding this comment.
@rjmholt FYI - just fixed the html encoding problem in the article. It will be live this afternoon.
Co-authored-by: Robert Holt <rjmholt@gmail.com> Co-authored-by: Steve Lee <slee@microsoft.com>
|
@SteveL-MSFT this is set to auto-merge, so when you refresh your review to an approval it will merge |
4 participants
PR Summary
Updated the README and Rules documentation.
PR Checklist
.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.