Skip to content
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

Fix broken links to old .md files, set correct root directory for Get-RuleDocumentationPath function #1730

Closed
wants to merge 4 commits into from

Conversation

fabge
Copy link

@fabge fabge commented Oct 14, 2021

PR Summary

There were still links to the old "RuleDocumentation" folder and some files were renamed.
The root directory if the Get-RuleDocumentationPath function was also set to the "Rules" folder

PR Checklist

@ghost
Copy link

ghost commented Oct 14, 2021

CLA assistant check
All CLA requirements met.

Copy link
Member

@andyleejordan andyleejordan left a comment

Choose a reason for hiding this comment

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

Great catch! One more extension to fix (I think).

- Must call ShouldProcess when ShouldProcess attribute is present and vice versa.[UseShouldProcess](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/RuleDocumentation/ShouldProcess.md)
- Nouns should be singular [UseSingularNouns](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/RuleDocumentation/UseSingularNouns.md)
- Module Manifest Fields [MissingModuleManifestField](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/RuleDocumentation/MissingModuleManifestField.md)
- Use Only Approved Verbs [UseApprovedVerbs](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/Rules/UseApprovedVerbs.cs)
Copy link
Contributor

Choose a reason for hiding this comment

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

The new links here will link to the source code. Instead they should link to the rule documentation, which looks like it now lives under https://github.com/PowerShell/PSScriptAnalyzer/tree/master/docs/Rules

Copy link
Member

@andyleejordan andyleejordan left a comment

Choose a reason for hiding this comment

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

Rob is right, the .cs are transformed into .md into the confusingly similarly named docs/Rules folder (instead of just Rules) found here: https://github.com/PowerShell/PSScriptAnalyzer/tree/master/docs/Rules

So, the links are still broken as-is, but the fix is different. Keep the .md extension, but point to docs/Rules/ instead of RuleDocumentation/ (or Rules/ as in this first iteration).

@rjmholt
Copy link
Contributor

rjmholt commented Oct 14, 2021

the confusingly similarly named docs/Rules folder

It used to be RulesDocumentation but #1724 changed this so all documentation is under one folder.

@fabge
Copy link
Author

fabge commented Oct 17, 2021

Thanks for the feedback. I pushed a new commit, changing the filepaths to the Markdown files under docs/Rules.
I couldn't find the corresponding Markdown file for the https://github.com/PowerShell/PSScriptAnalyzer/blob/master/Rules/AvoidAlias.cs rule. Any pointers if this is missing or does not exist?

Copy link
Member

@andyleejordan andyleejordan left a comment

Choose a reason for hiding this comment

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

Thanks so much!

- No Invoke-Expression [AvoidUsingInvokeExpression](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/Rules/AvoidUsingInvokeExpression.cs)
- Avoid using alias [AvoidAlias](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/docs/Rules/AvoidAlias.md) #TODO not found
- Avoid using deprecated WMI cmdlets [AvoidUsingWMICmdlet](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/docs/Rules/AvoidUsingWMICmdlet.md)
- Empty catch block should not be used [AvoidEmptyCatchBlock](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/docs/Rules/AvoidUsingEmptyCatchBlock.md)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Empty catch block should not be used [AvoidEmptyCatchBlock](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/docs/Rules/AvoidUsingEmptyCatchBlock.md)
- Empty catch block should not be used [AvoidUsingEmptyCatchBlock](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/docs/Rules/AvoidUsingEmptyCatchBlock.md)

- Empty catch block should not be used [AvoidEmptyCatchBlock](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/docs/Rules/AvoidUsingEmptyCatchBlock.md)
- Invoke existing cmdlet with correct parameters [UseCmdletCorrectly](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/docs/Rules/UseCmdletCorrectly.md)
- Cmdlets should have ShouldProcess/ShouldContinue and Force param if certain system-modding verbs are present (Update, Set, Remove, New): [UseShouldProcessForStateChangingFunctions](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/docs/Rules/UseShouldProcessForStateChangingFunctions.md)
- Positional parameters should be avoided [AvoidPositionalParameters](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/docs/Rules/AvoidUsingPositionalParameters.md)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Positional parameters should be avoided [AvoidPositionalParameters](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/docs/Rules/AvoidUsingPositionalParameters.md)
- Positional parameters should be avoided [AvoidUsingPositionalParameters](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/docs/Rules/AvoidUsingPositionalParameters.md)


### Severity: Warning

- Password = 'string' should not be used. (information disclosure) [AvoidUserNameAndPasswordParams](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/Rules/AvoidUserNameAndPasswordParams.cs)
- Password = 'string' should not be used. (information disclosure) [AvoidUserNameAndPasswordParams](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/docs/Rules/AvoidUsingUsernameAndPasswordParams.md)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Password = 'string' should not be used. (information disclosure) [AvoidUserNameAndPasswordParams](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/docs/Rules/AvoidUsingUsernameAndPasswordParams.md)
- Password = 'string' should not be used. (information disclosure) [AvoidUsingUserNameAndPasswordParams](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/docs/Rules/AvoidUsingUsernameAndPasswordParams.md)

- Global variables should be avoided. [AvoidGlobalVars](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/Rules/AvoidGlobalVars.cs)
- Declared variables must be used in more than just their assignment. [UseDeclaredVarsMoreThanAssignments](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/Rules/UseDeclaredVarsMoreThanAssignments.cs)
- No Invoke-Expression [AvoidUsingInvokeExpression](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/Rules/AvoidUsingInvokeExpression.cs)
- Avoid using alias [AvoidAlias](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/docs/Rules/AvoidAlias.md) #TODO not found
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Avoid using alias [AvoidAlias](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/docs/Rules/AvoidAlias.md) #TODO not found
- Avoid using cmdlet aliases [AvoidUsingCmdletAliases](https://github.com/PowerShell/PSScriptAnalyzer/blob/master/docs/Rules/AvoidUsingCmdletAliases.md)

I looked through the Git history and this was renamed in 11a04a9

Copy link
Collaborator

@bergmeister bergmeister left a comment

Choose a reason for hiding this comment

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

LGTM, I resolved the merge conflict, please check you are happy with it. Can you give it a final review @sdwheeler and @andschwa please?

@sdwheeler
Copy link
Collaborator

Just a note about this article. This has been migrated to https://docs.microsoft.com/powershell/utility-modules/psscriptanalyzer/rules-recommendations. It is scheduled to be deleted from the source repo in a month.

@fabge
Copy link
Author

fabge commented Apr 7, 2022

Guys, can you either merge or reject? I'd like this closed...

@andyleejordan
Copy link
Member

@SydneyhSmith and @JamesWTruher we need to triage / merge this!

@bergmeister
Copy link
Collaborator

To me it sounds like we can close it since it's moved to an upstream source and the files here will vanish soon. Do you agree @sdwheeler ?

@sdwheeler
Copy link
Collaborator

@bergmeister I agree. However, there is still the change to Utils/RuleMaker.psm1 that needs to be made. If we merge this then we get that change. The other doc will be delete in a month so the changes to it can be merged as is. The doc is already published to the live site at https://docs.microsoft.com/powershell/utility-modules/psscriptanalyzer/rules-recommendations. So the broken links are not an issue.

This pull request was closed.
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.

5 participants