Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Merged
daxian-dbw merged 2 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
PetSerAl:recursive-array-fixCopy head branch name to clipboard
Feb 27, 2017
Merged

Fix powershell to not crash on converting recursive array to bool#3208
daxian-dbw merged 2 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
PetSerAl:recursive-array-fixCopy head branch name to clipboard

Conversation

@PetSerAl

@PetSerAl PetSerAl commented Feb 26, 2017

Copy link
Copy Markdown
Contributor

Fixing #3207

@msftclas

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

@lzybkr

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.

@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

@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

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

@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

@PetSerAl

PetSerAl commented Feb 27, 2017

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

@daxian-dbw

daxian-dbw commented Feb 27, 2017

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

@lzybkr

lzybkr commented Feb 27, 2017

Copy link
Copy Markdown
Contributor

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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.