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 typo in example #1379

Merged
merged 2 commits into from
Dec 3, 2019
Merged

Fix typo in example #1379

merged 2 commits into from
Dec 3, 2019

Conversation

Farwaykorse
Copy link
Contributor

Correct the script name to match the name in the example.

Removed the when using. It belonged with the when using Invoke-ScriptAnalyzer used in the previous example and repeating that seems unnecessary.

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.

Thanks for making it consistent.
I am happy with the proposed change but I think we could improve it further by making 2 more changes:

  • Use the full parameter name, which is -Settings in both examples
  • Replace PSScriptAnalyzerSettings.psd1 in the 2 examples with $PSScriptAnalyzerSettingsPath. This is because using -Settings PSScriptAnalyzerSettings.psd1 is redundant due to the implicit settings feature.

Let me know what you think.

@Farwaykorse
Copy link
Contributor Author

I agree with the use of -Settings.

I wouldn't use the variable, it just adds confusion over the meaning of the variable.
e.g. When you wrote it, my first assumption was that it was a variable defined by PSScriptAnalyzer.

The current use of PSScriptAnalyzerSettings.psd1 as a filename shows the examples as they apply to both the implicit use and by user supplied path.

@bergmeister
Copy link
Collaborator

Ok, happy with that as well, it is definitely an improvement. Thanks 😀

@bergmeister bergmeister merged commit 20c4611 into PowerShell:master Dec 3, 2019
@Farwaykorse Farwaykorse deleted the patch-2 branch December 3, 2019 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants