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 — Fix powershell to not crash on converting recursive array to bool by PetSerAl · Pull Request #3208 · PowerShell/PowerShell

Conversation

@PetSerAl

@PetSerAl PetSerAl commented Feb 26, 2017 •
edited by daxian-dbw
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Fixing #3207

Sorry, something went wrong.

Uh oh!

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

All reactions
PetSerAl added 2 commits February 26, 2017 22:59
@PetSerAl
Add failing test
074c76a
@PetSerAl
Fixes #3207
448e6d8
@msftclas

Microsoft Contribution License Agreements (msftclas) commented Feb 26, 2017

Copy link
Copy Markdown

@PetSerAl,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
Microsoft Pull Request Bot

All reactions

Sorry, something went wrong.

Uh oh!

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

@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-required label Feb 26, 2017
lzybkr
Jason Shirk (lzybkr) approved these changes Feb 27, 2017
View reviewed changes
@lzybkr Jason Shirk (lzybkr) assigned Dongbo Wang (daxian-dbw) Feb 27, 2017
@lzybkr

Jason Shirk (lzybkr) commented Feb 27, 2017

Copy link
Copy Markdown
Contributor

Code changes look good - we do need the cla to be signed before it can be merged.

All reactions

Sorry, something went wrong.

Uh oh!

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

@daxian-dbw Dongbo Wang (daxian-dbw) changed the title Fixes issue #3207 Fix powershell to not crash on converting recursive array to bool Feb 27, 2017
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Feb 27, 2017

Copy link
Copy Markdown
Member

PetSerAl Thanks for your contribution!
I updated the title to reflect the change. It's recommended to have a meaningful PR title describing what changes you made (see the guideline in CONTRIBUTING.md).

All reactions

Sorry, something went wrong.

Uh oh!

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

@msftgits

Microsoft GitHub User (msftgits) commented Feb 27, 2017

Copy link
Copy Markdown

Hi, I am closing and re-opening this PR to bump the CLA bot. Sorry for the inconvenience!

All reactions

Sorry, something went wrong.

Uh oh!

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

@msftgits Microsoft GitHub User (msftgits) closed this Feb 27, 2017
@msftgits Microsoft GitHub User (msftgits) reopened this Feb 27, 2017
@msftclas

Microsoft Contribution License Agreements (msftclas) commented Feb 27, 2017

Copy link
Copy Markdown

PetSerAl,
Thanks for having already signed the Contribution License Agreement. Your agreement has not been validated yet. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

All reactions

Sorry, something went wrong.

Uh oh!

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

@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-signed label Feb 27, 2017
@msftgits Microsoft GitHub User (msftgits) removed the cla-required label Feb 27, 2017
@daxian-dbw
Dongbo Wang (daxian-dbw) merged commit ae37a9f into PowerShell:master Feb 27, 2017
@daxian-dbw Dongbo Wang (daxian-dbw) mentioned this pull request Feb 27, 2017
PowerShell process crash on recursive array converted to bool #3207
Closed
@PetSerAl

PetSerAl commented Feb 27, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

One possible breaking change came to my mind:

$a = ,,$false
$b = ,[PSObject](,$false)
!!$a # $true
!!$b # was $false, now is $true

So, this PR make PowerShell behavior more consistent in regard to wrapping array into [PSObject]. But, is this change in behavior acceptable, Dongbo Wang (@daxian-dbw)?

All reactions

Sorry, something went wrong.

Uh oh!

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

@daxian-dbw

Dongbo Wang (daxian-dbw) commented Feb 27, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

I think the behavior with the fix reflects the original intention of the code. Here is the comment in "case 1" before returning true:

// the first element is an array with more than zero elements
return true;

And as you said, now the conversion is more consistent:
Before Fix

$true -eq $a
True
$true -eq $b
False

After Fix

$true -eq $a
True
$true -eq $b
True

Using a nested array in if condition should be rare. I think this behavior change is acceptable. Jason Shirk (@lzybkr) any thoughts?

All reactions

Sorry, something went wrong.

Uh oh!

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

@lzybkr

Jason Shirk (lzybkr) commented Feb 27, 2017

Copy link
Copy Markdown
Contributor

It's definitely obscure, I'm not worried about the change.

All reactions

Sorry, something went wrong.

Uh oh!

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

@PetSerAl
PetSerAl deleted the recursive-array-fix branch March 19, 2017 00:02
@mklement0 Michael Klement (mklement0) mentioned this pull request Sep 16, 2018
Start-Process -WindowStyle Minimized steals focus #7790
Closed
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@PetSerAl @daxian-dbw
Fix powershell to not crash on converting recursive array to bool (Po…
4527cf1
…werShell#3208)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@lzybkr Jason Shirk (lzybkr) lzybkr approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

@daxian-dbw Dongbo Wang (daxian-dbw)

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.

5 participants

@PetSerAl @msftclas @lzybkr @daxian-dbw @msftgits
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.