New rule: AvoidOverwritingBuiltInCmdlets by thomasrayner · Pull Request #1348 · PowerShell/PSScriptAnalyzer
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 ArgumentNullException that likely comes from your rule as it is being run by default. What kind of performance impact does running this rule by default have (cold and warm run)? I've just merged your other PR, so please resolve the merge conflict as well (and you probably need to re-generate the resource strings as well).
The JSON files that you use are from an old compatibility rule, it is probably OK for this use case but maybe @rjmholt can make a statement whether his compatibility profiles contain more commands? They are here btw: https://github.com/PowerShell/PSScriptAnalyzer/tree/master/PSCompatibilityCollector/profiles
@rjmholt can make a statement whether his compatibility profiles contain more commands?
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...
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 😂, contrary to my original thought. The four remaining tests that fail are for the UseCompatibleCommands rule test, and only appear to occur on the Ubuntu test env.
@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).
Most of it looks ok but I can already tell you we'll need to have a conversation around that test process stuff (sorry, but just wrong on so many levels IMHO). First of all, the user should be explicit in the settings and second, this is how you can determine in which version of PowerShell you are currently running in order to determine a useful default
bool runningInPowerShellCore = false; #if CORECLR runningInPowerShellCore = true; #endif
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
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
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 ?
Christoph Bergmeister added 2 commits
November 19, 2019 19:29I'm happy, what about you @rjmholt ? I'd merge it if you're happy.
You don't need to do a full review, just checking you're happy with it on a high level.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Still looks good to me, I resolved the merge conflict. The test failures on Ubuntu are new in master as well @rjmholt ...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters