- Notifications
You must be signed in to change notification settings - Fork407
New rule: AvoidOverwritingBuiltInCmdlets#1348
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
New rule: AvoidOverwritingBuiltInCmdlets#1348
Uh oh!
There was an error while loading.Please reload this page.
Conversation
thomasrayner commentedOct 2, 2019
It appears that a whole bunch of tests are busted but I'm not sure it's a result of the changes in this PR, however, I haven't dug that deep into them on the assumption that this is already known. |
bergmeister commentedOct 4, 2019
Please look into the test failures, they are definitely caused by your code as I see a lot of exceptions of type |
rjmholt commentedOct 5, 2019
If it's just builtin commands, I would imagine the old profiles have the same information (a list of those commands). Depending on the full compatibility profiles would be a resource drain. In a case like this, since you only need a list, either using those old profiles or just storing a list in the rule's code will probably suffice for now. |
rjmholt commentedOct 5, 2019
Ah but I just saw it's a configuration thing... Hmmm, not sure what the right answer is there, since it's public, so we don't want to break it in future. Those old profiles aren't about to get any updates as far as I know, so it's not ideal to depend on them publicly. It might be that it's time to solve the question of how to specify a PowerShell platform nicely... |
bergmeister commentedOct 5, 2019
The reason why those old command data files contain less cmdlets is because of the way they are generated here:https://github.com/PowerShell/PSScriptAnalyzer/blob/master/Utils/New-CommandDataFile.ps1 |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
1d46ca4 toaeea36cCompare07825ce to6fb31b6Comparethomasrayner commentedOct 16, 2019
Obviously I've got more to do here, like dig into those test failures, but I wanted to get some of these changes back to you folks to discuss. |
thomasrayner commentedOct 23, 2019
I took a shot at fixing the test errors that were definitely my fault 😂, contrary to my original thought. The four remaining tests that fail are for the |
bergmeister commentedOct 23, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@thomasrayner Thanks, I will look at the PR this weekend. You are right, the 4 failing Ubuntu tests are a new integration failure on master caused by the last merged PR#1331 (@rjmholt please fix them please). The sporadically failing test around runspace disposal, you can ignore as well (the maximum number needs increasing). boolrunningInPowerShellCore=false;#ifCORECLRrunningInPowerShellCore=true;#endif |
thomasrayner commentedOct 23, 2019
Well, I'm not sure how I missed that, but I'm looking forward to changing it 😁 |
bergmeister left a comment
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.
Just some minor comments to be discussed but otherwise I'm pretty happy with it and I've done a quick performance test and can say that this does not seem to have an impact on PSSA performance despite it being enabled by default
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
bergmeister left a comment
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.
Some more minor comments but after that I'm happy to sign it off. What do you think@rjmholt
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-Authored-By: Christoph Bergmeister [MVP] <c.bergmeister@gmail.com>
bergmeister left a comment
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'm happy, what about you@rjmholt ?
bergmeister commentedDec 3, 2019
I'm happy, what about you@rjmholt ? I'd merge it if you're happy. |
rjmholt left a comment
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
bergmeister commentedDec 9, 2019
Still looks good to me, I resolved the merge conflict. The test failures on Ubuntu are new in master as well@rjmholt ... |
PR Summary
Addresses#1023 .
This rule (PSAvoidOverwritingBuiltInCmdlets) looks for
FunctionDefinitionAsts that have the same name as a cmdlet that comes with PowerShell. This rule leverages the JSON files which contain information on all the cmdlets for PS 3, 4, 5, and 6 (and presumably 7 later) that already ship with PSScriptAnalzyer.I've editedGetScriptAnalyzerRule.tests.ps1 to reflect the addition of this new rule, but suspect that ifthe other rule I recently created a PR for is merged before this one, that I'll need to edit that test again.
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.