-
Notifications
You must be signed in to change notification settings - Fork 498
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
Updates to Examples PSSA settings file to include more rule config #1312
Updates to Examples PSSA settings file to include more rule config #1312
Conversation
@bergmeister Could you take a quick look at this? This is for the Examples folder we ship with VSCode. I've updated the PSSA settings file in it to include info on various rule configurations. |
# diagnostic record for this file due to a bug - | ||
# https://github.com/PowerShell/PSScriptAnalyzer/issues/472 | ||
# For more information on PSScriptAnalyzer settings see: | ||
# https://github.com/PowerShell/PSScriptAnalyzer/blob/master/README.md#settings-support-in-scriptanalyzer |
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.
Also linking to the shipped settings files (as real examples) could be useful: https://github.com/PowerShell/PSScriptAnalyzer/tree/master/Engine/Settings
# Placement = "before" | ||
# } | ||
# PSUseCompatibleCmdlets = @{ | ||
# compatibility = @("core-6.0.0-alpha-windows", "core-6.0.0-alpha-linux") |
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.
I want to merge this PR before the release next week, which will delete those files and replace it with newer ones. Therefore I would remove this example, you do not need to document PSSA details.
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.
I'd much prefer a link to a list of configurable PSSA rules but I didn't see one. I spent ~10 minutes going through every single rule doc, looking for the ones that are configurable. I think it would be nice if PSSA had a list of configurable rules that could be linked to. :-)
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.
Get-ScriptAnalyzer
gives you all the rules and one has to look at each rule doc for configuration. The plan is to include one settings file in the PSSA repo itself with all rules and options being present. I had the same problem when doing it the first time for me a few months ago and had only the rule docs and example files. Since I didn't write the tool, I can also only use as much as currently is there. I learn more about it as I read the code and improve docs as I go along.
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.
My point is that somewhere in this README it would be helpful to have a list of the configurable rules.
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.
I have deleted this ReadMe already in the development
branch because it got too much out of date and there is a lot of duplicated and outdated information on the repo. Why do you want to know whether a rule is configurable? The idea would be that one takes the monolithic example file with all rules and configuration examples and then one deletes the rules that one is not interested in and adapt the configuration of the remaining rules.
There is an outstanding issue to get all configurable rules using Get-ScriptAnalyzer
and I remember giving it a quick try in an attempted PR and there were various problems, hence why I left the issue for the moment: PowerShell/PSScriptAnalyzer#823
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.
Ah, that's too bad. I just submitted a PR to fix the broken links and add a column for Configurable
. Because I'm too lazy and don't have enough time to read through all the docs. If a rule isn't working for me, I'll just disable it. OTOH if I saw that the rule was configurable it wouldn't take me long to make it work for me, then I'd leave it enabled.
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.
I would maybe remove the code formatting related settings and the PSUseCompatibleCmdlets
rule from the example file, this should be part of PSSA docs (I plan to write a blog with VSCode and PSSA integration and customisation soon because this is a topic that comes up often). Also, for most people, it is going to be better to just configure the powershell.codeFormatting.preset
vscode setting for auto-formatting and only run code-analysis, not style rules against the code (I have personally tried it and found even slightly varying behaviour of rule results when comparing Invoke-Formatter
to Invoke-ScriptAnalyzer
)
Also, to me, it would also be important to document that the powershell.scriptAnalysis.settingsPath
setting of vscode is linked to this PSSA settings file (that will be part of the blog and some PSSA PRs to eat some dog food and show real examples).
OK, so this PR title should now be - "Improved documentation & links in the Examples PSSA settings file". |
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.
Looks OK, but line 17 has an existing spelling error (defualt
).
Once I have more docs and examples in PSSA, I would like to only keep the top section with links to PSSA docs but this is something for the future.
@bergmeister Thanks for taking the time to review this! |
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.
LGTM! Thanks Keith 😄
No description provided.