Skip to content
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

Fix logic errors in AvoidAlias rule #1251

Merged
merged 2 commits into from Jun 5, 2019

Conversation

@JamesWTruher
Copy link
Member

JamesWTruher commented Jun 5, 2019

PR Summary

The first fix is that after the yield return, we should move on to the next command
The second fix is that if we find any command, we should not move on to the 'Get-' variant.
These changes mean that we do not run a pipeline (twice!) to find something that was a cache miss. Previously in the pathological case of a script made up of only unique aliases, we would run 2 pipelines for each found alias. This fix avoids both of those pipeline invocations.

PR Checklist

The first fix is that after the yield return, we should move on to the next command
The second fix is that if we find _any_ command, we should not move on to the 'Get-<command>' variant
These changes mean that we do not run a pipeline (twice!) to find something that was a cache miss. In the pathological case of a script mmade up of only unique aliases, we would run 2 pipelines for each found alias.
@JamesWTruher JamesWTruher requested a review from bergmeister Jun 5, 2019
Rules/AvoidAlias.cs Outdated Show resolved Hide resolved
Copy link
Collaborator

bergmeister left a comment

One minor suggestion but looks great otherwise. I will modify PR #1178 to not parallelize the AvoidAlias rule any more since with this improvement the rule is no longer the weakest link in the chain

Co-Authored-By: Christoph Bergmeister [MVP] <c.bergmeister@gmail.com>
@JamesWTruher JamesWTruher merged commit 9d19209 into PowerShell:development Jun 5, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.