FFFF Fix powershell to not crash on converting recursive array to bool by PetSerAl · Pull Request #3208 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

Fix powershell to not crash on converting recursive array to bool#3208

Merged
daxian-dbw merged 2 commits intoPowerShell:masterfrom
PetSerAl:recursive-array-fix
Feb 27, 2017
Merged

Fix powershell to not crash on converting recursive array to bool#3208
daxian-dbw merged 2 commits intoPowerShell:masterfrom
PetSerAl:recursive-array-fix

Conversation

@PetSerAl
Copy link
Copy Markdown
Contributor
@PetSerAl PetSerAl commented Feb 26, 2017 •
edited by daxian-dbw
Loading

Fixing #3207

@msftclas
Copy link
Copy Markdown
msftclas commented Feb 26, 2017

@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

@lzybkr
Copy link
Copy Markdown
Contributor
lzybkr commented Feb 27, 2017

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

@daxian-dbw daxian-dbw changed the title Fixes issue #3207 Fix powershell to not crash on converting recursive array to bool Feb 27, 2017
@daxian-dbw
Copy link
Copy Markdown
Member
daxian-dbw commented Feb 27, 2017

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

@msftgits
Copy link
Copy Markdown
msftgits commented Feb 27, 2017

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

@msftgits msftgits closed this Feb 27, 2017
@msftgits msftgits reopened this Feb 27, 2017
@msftclas
Copy link
Copy Markdown
msftclas commented Feb 27, 2017

@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

@daxian-dbw daxian-dbw merged commit ae37a9f into PowerShell:master Feb 27, 2017
@daxian-dbw daxian-dbw mentioned this pull request Feb 27, 2017
Closed
@PetSerAl
Copy link
Copy Markdown
Contributor Author
PetSerAl commented Feb 27, 2017 •
edited
Loading

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, @daxian-dbw?

@daxian-dbw
Copy link
Copy Markdown
Member
daxian-dbw commented Feb 27, 2017 •
edited
Loading

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. @lzybkr any thoughts?

@lzybkr
Copy link
Copy Markdown
Contributor
lzybkr commented Feb 27, 2017

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

@PetSerAl PetSerAl deleted the recursive-array-fix branch March 19, 2017 00:02
@mklement0 mklement0 mentioned this pull request Sep 16, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@lzybkr lzybkr lzybkr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@daxian-dbw daxian-dbw

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@PetSerAl @msftclas @lzybkr @daxian-dbw @msftgits
0