8000 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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 27, 2017
Merged

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

merged 2 commits into from
Feb 27, 2017

Conversation

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

Fixing #3207

@msftclas
Copy link

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

@msftgits
Copy link

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

@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
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
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
Contributor
lzybkr commented Feb 27, 2017

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

@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

@lzybkr lzybkr lzybkr approved these changes

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