Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upNew rule: AvoidOverwritingBuiltInCmdlets #1348
Conversation
|
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. |
|
Please look into the test failures, they are definitely caused by your code as I see a lot of exceptions of type |
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. |
|
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... |
|
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 |
| } | ||
|
|
||
|
|
||
| private HashSet<string> GetCmdletsFromData(dynamic deserializedObject) |
This comment has been minimized.
This comment has been minimized.
rjmholt
Oct 5, 2019
Member
This is the other disadvantage of the old profiles; they don't have a formalised schema. The more efficient way to do this would be to get the JObject instead though.
This comment has been minimized.
This comment has been minimized.
thomasrayner
Oct 16, 2019
Author
Contributor
@rjmholt do you recommend I change it? Perpetuating legacy practices isn't high on my to-do list, but I'm wondering how far removed from the desired practice it is.
1d46ca4
to
aeea36c
07825ce
to
6fb31b6
|
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. |
|
I took a shot at fixing the test errors that were definitely my fault |
|
@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). bool runningInPowerShellCore = false;
#if CORECLR
runningInPowerShellCore = true;
#endif |
|
Well, I'm not sure how I missed that, but I'm looking forward to changing it |
|
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 |
|
Some more minor comments but after that I'm happy to sign it off. What do you think @rjmholt |
|
I'm happy, what about you @rjmholt ? |
|
I'm happy, what about you @rjmholt ? I'd merge it if you're happy. |
|
LGTM |
|
Still looks good to me, I resolved the merge conflict. The test failures on Ubuntu are new in master as well @rjmholt ... |
thomasrayner commentedOct 2, 2019
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 edited GetScriptAnalyzerRule.tests.ps1 to reflect the addition of this new rule, but suspect that if the 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.