GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — New rule: AvoidOverwritingBuiltInCmdlets by thomasrayner · Pull Request #1348 · PowerShell/PSScriptAnalyzer

Conversation

@thomasrayner

@thomasrayner Thomas Rayner (thomasrayner) commented Oct 2, 2019

Copy link
Copy Markdown
Contributor

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

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Change is not breaking
  • Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
  • Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Thomas Rayner (thomasrayner) added 3 commits October 2, 2019 10:03
@thomasrayner
avoidoverwritingbuiltincmdlets first draft
05f6c3d
@thomasrayner
rough draft avoidoverwritingcmdlets working
8aa60a9
@thomasrayner
Added tests, fixed typos, changed default PowerShellVersion behavior
651ed5d
@thomasrayner

Thomas Rayner (thomasrayner) commented Oct 2, 2019

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bergmeister

Christoph Bergmeister (bergmeister) commented Oct 4, 2019

Copy link
Copy Markdown
Collaborator

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 Rob Holt (@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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rjmholt

Rob Holt (rjmholt) commented Oct 5, 2019

Copy link
Copy Markdown
Contributor

Rob Holt (@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rjmholt

Rob Holt (rjmholt) commented Oct 5, 2019

Copy link
Copy Markdown
Contributor

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...

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bergmeister

Christoph Bergmeister (bergmeister) commented Oct 5, 2019

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rjmholt
Rob Holt (rjmholt) reviewed Oct 5, 2019
View reviewed changes
Comment thread Rules/AvoidOverwritingBuiltInCmdlets.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Rules/AvoidOverwritingBuiltInCmdlets.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Rules/AvoidOverwritingBuiltInCmdlets.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Rules/AvoidOverwritingBuiltInCmdlets.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Rules/AvoidOverwritingBuiltInCmdlets.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Rules/AvoidOverwritingBuiltInCmdlets.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Rules/AvoidOverwritingBuiltInCmdlets.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Rules/AvoidOverwritingBuiltInCmdlets.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Rules/AvoidOverwritingBuiltInCmdlets.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Rules/AvoidOverwritingBuiltInCmdlets.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

@thomasrayner
updates a/p rjmholt
aeea36c
@thomasrayner
Thomas Rayner (thomasrayner) force-pushed the avoidoverwritingcmdlets branch from 1d46ca4 to aeea36c Compare October 16, 2019 20:51
Thomas Rayner (thomasrayner) added 6 commits October 16, 2019 14:05
@thomasrayner
remove unneeded else
07825ce
@thomasrayner
avoidoverwritingbuiltincmdlets first draft
4583924
@thomasrayner
rough draft avoidoverwritingcmdlets working
9c90ccd
@thomasrayner
Added tests, fixed typos, changed default PowerShellVersion behavior
f5c6a9d
@thomasrayner
updates a/p rjmholt
cedb19b
@thomasrayner
remove unneeded else
6fb31b6
@thomasrayner
Thomas Rayner (thomasrayner) force-pushed the avoidoverwritingcmdlets branch from 07825ce to 6fb31b6 Compare October 16, 2019 21:09
@thomasrayner
changes from upstream
e1bfadd
@thomasrayner

Thomas Rayner (thomasrayner) commented Oct 16, 2019

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Thomas Rayner (thomasrayner) added 6 commits October 23, 2019 08:51
@thomasrayner
updated readme - want tests to run in CI again
c55369c
@thomasrayner
prevent adding duplicate keys
a67d3e9
@thomasrayner
return an empty list instead of null
bb6ae0a
@thomasrayner
update rule count
debee2e
@thomasrayner
fixing pwsh not present issue in test
ee3bb2e
@thomasrayner
fixing a ps 4 test broke a linux test
e1688af
@thomasrayner

Thomas Rayner (thomasrayner) commented Oct 23, 2019

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bergmeister

Christoph Bergmeister (bergmeister) commented Oct 23, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

Thomas Rayner (@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 (Rob Holt (@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
thomasrayner reacted with laugh emoji thomasrayner reacted with heart emoji
All reactions
  • 😄 1 reaction
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@thomasrayner

Thomas Rayner (thomasrayner) commented Oct 23, 2019

Copy link
Copy Markdown
Contributor Author

Well, I'm not sure how I missed that, but I'm looking forward to changing it 😁

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@thomasrayner
better PS core detection
c9bb6b6
bergmeister
Christoph Bergmeister (bergmeister) reviewed Oct 26, 2019
View reviewed changes

@bergmeister Christoph Bergmeister (bergmeister) left a comment

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread RuleDocumentation/AvoidOverwritingBuiltInCmdlets.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Rules/AvoidOverwritingBuiltInCmdlets.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Rules/AvoidOverwritingBuiltInCmdlets.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Rules/Strings.resx Resolved

Uh oh!

There was an error while loading. Please reload this page.

Thomas Rayner (thomasrayner) added 2 commits October 28, 2019 13:59
@thomasrayner
Add reference to UseCompatibleCmdlets doc
59e11d8
@thomasrayner
changes a/p Chris
111495d
bergmeister
Christoph Bergmeister (bergmeister) reviewed Oct 28, 2019
View reviewed changes

@bergmeister Christoph Bergmeister (bergmeister) left a comment

Copy link
Copy Markdown
Collaborator

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 Rob Holt (@rjmholt)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread RuleDocumentation/AvoidOverwritingBuiltInCmdlets.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread RuleDocumentation/AvoidOverwritingBuiltInCmdlets.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Rules/AvoidOverwritingBuiltInCmdlets.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Thomas Rayner (thomasrayner) and others added 2 commits October 29, 2019 08:13
@thomasrayner @bergmeister
Update RuleDocumentation/AvoidOverwritingBuiltInCmdlets.md
da05f30
Co-Authored-By: Christoph Bergmeister [MVP] <[email protected]>
@thomasrayner
trimmed doc and changed functiondefinitions detection to be more perf…
fbd5ba0
…ormant
bergmeister
Christoph Bergmeister (bergmeister) approved these changes Nov 19, 2019
View reviewed changes

@bergmeister Christoph Bergmeister (bergmeister) left a comment

Copy link
Copy Markdown
Collaborator

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 Rob Holt (@rjmholt) ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Christoph Bergmeister added 2 commits November 19, 2019 19:29
Christoph Bergmeister
retrigger-ci after fix was made in master
f2df044
Christoph Bergmeister
retrigger-ci due to sporadic test failure
c2961eb
@bergmeister

Christoph Bergmeister (bergmeister) commented Dec 3, 2019

Copy link
Copy Markdown
Collaborator

I'm happy, what about you Rob Holt (@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rjmholt
Rob Holt (rjmholt) approved these changes Dec 9, 2019
View reviewed changes

@rjmholt Rob Holt (rjmholt) left a comment

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Christoph Bergmeister (bergmeister) added 2 commits December 9, 2019 22:03
@bergmeister
Update number of expected rules due to recent merge of PR PowerShell#…
5a8e109
…1373
@bergmeister
Merge branch 'master' into avoidoverwritingcmdlets
bdd7dbe
@bergmeister

Christoph Bergmeister (bergmeister) commented Dec 9, 2019

Copy link
Copy Markdown
Collaborator

Still looks good to me, I resolved the merge conflict. The test failures on Ubuntu are new in master as well Rob Holt (@rjmholt) ...

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bergmeister
Christoph Bergmeister (bergmeister) merged commit ab69069 into PowerShell:master Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bergmeister Christoph Bergmeister (bergmeister) bergmeister approved these changes
+2 more reviewers
@kilasuit Ryan Yates (kilasuit) kilasuit left review comments
@rjmholt Rob Holt (rjmholt) rjmholt approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@thomasrayner @bergmeister @rjmholt @kilasuit
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.