GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Fix casting single element array to generic collection by daxian-dbw · Pull Request #3170 · PowerShell/PowerShell

Conversation

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

Copy link

Copy Markdown

Member

Fix #2208

Sorry, something went wrong.

Uh oh!

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

All reactions


          Fix casting single element array to generic collection

658883a

Dongbo Wang (daxian-dbw) assigned Jason Shirk (lzybkr)

Feb 19, 2017

Dongbo Wang (daxian-dbw) requested a review from Jason Shirk (lzybkr)

February 19, 2017 05:48

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label

Feb 19, 2017

Ilya (iSazonov) reviewed

Feb 19, 2017

View reviewed changes

Comment thread

test/powershell/Language/Parser/Conversions.Tests.ps1 Outdated

		for ($i = 0; $i -lt $Count; $i++)
		{
		$result[$i] \| Should Be $Elements[$i]
		}

Ilya (iSazonov) Feb 19, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use Compare-Object and exclude Count at all?

Sorry, something went wrong.

Uh oh!

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

All reactions

Dongbo Wang (daxian-dbw) Feb 20, 2017 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the Count key is not necessary, and I updated the tests to avoid it.
Instead of using a loop or Compare-Object, I think $result -join ";" | Should Be ($Elements -join ";") is better here. In case the results are different, I can see the whole set of results and expected values in the log.

Sorry, something went wrong.

Uh oh!

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

All reactions

Ilya (iSazonov) Feb 21, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
Closed.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

test/powershell/Language/Parser/Conversions.Tests.ps1 Outdated


		@{ Command = {$result = [System.Collections.ObjectModel.Collection[int]]@(1)}; CollectionType = 'Collection`1'; ElementType = "Int32"; Count = 1; Elements = @(1) }
		@{ Command = {$result = [System.Collections.ObjectModel.Collection[int]]@(1,2)}; CollectionType = 'Collection`1'; ElementType = "Int32"; Count = 2; Elements = @(1,2) }
		@{ Command = {$result = [System.Collections.ObjectModel.Collection[int]]"4"}; CollectionType = 'Collection`1'; ElementType = "Int32"; Count = 1; Elements = @(4) }

Ilya (iSazonov) Feb 19, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it makes sense to add tests for complex type items (array, hash, psobject) because code use LanguagePrimitives.ConvertTo?

Sorry, something went wrong.

Uh oh!

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

All reactions

Dongbo Wang (daxian-dbw) Feb 20, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added one more set of tests for [List[FileInfo]] cast from string or array of strings. If you think more tests is needed in this area, feel free to open an issue.

Sorry, something went wrong.

Uh oh!

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

All reactions

Ilya (iSazonov) Feb 21, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose that's enough.
Closed.

Sorry, something went wrong.

Uh oh!

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

All reactions

PowerShellTeam added the Review - Needed The PR is being reviewed label

Feb 20, 2017

Dongbo Wang (daxian-dbw) added 2 commits

February 20, 2017 15:17


          Fix comments in tests

b8cf316


          Fix the test a bit more

Ilya (iSazonov) reviewed

Feb 21, 2017

View reviewed changes

Comment thread

test/powershell/Language/Parser/Conversions.Tests.ps1 Outdated

		$result -join ";" \| Should Be ($Elements -join ";")
		}

		It "<Command>" -TestCases $testCases2 {

Ilya (iSazonov) Feb 21, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we split testCases1 and testCases2? It seems the test code is the same.

Sorry, something went wrong.

Uh oh!

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

All reactions

Dongbo Wang (daxian-dbw) Feb 21, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point :) I thought the FileInfo test needs to be @($result.Name) -join ";", but $result -join ";" should work too.

Sorry, something went wrong.

Uh oh!

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

All reactions


          One more fix in the test

8d0907c

Dongbo Wang (daxian-dbw) mentioned this pull request

Feb 21, 2017

Define roles and their responsibilities in a PR #3168

Merged

Ilya (iSazonov) commented Feb 22, 2017

Copy link

Copy Markdown

Collaborator

LGTM for tests.

Dongbo Wang (@daxian-dbw) Please clarify for me: Is the PR related with return ,$arrayVar ?

All reactions

Sorry, something went wrong.

Uh oh!

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

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

Copy link

Copy Markdown

Member Author

Ilya (@iSazonov) this PR is not related to the return ,$array approach to prevent unraveling the output. This PR is only for fixing #2208, so that you can cast a one-element array to a generic collection.

All reactions

Sorry, something went wrong.

Uh oh!

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

Ilya (iSazonov) commented Feb 22, 2017

Copy link

Copy Markdown

Collaborator

Dongbo Wang (@daxian-dbw) Thanks for clarify!

All reactions

Sorry, something went wrong.

Uh oh!

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

Jason Shirk (lzybkr) reviewed

Feb 22, 2017

View reviewed changes

Jason Shirk (lzybkr) left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have one comment on the code - it would be good to ensure you know what the regression was from V4 and that we have tests for it - so we ensure this fix (which looks right to me) doesn't regress anything else.

Other than that, just the 2 minor comments on the tests.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

test/powershell/Language/Parser/Conversions.Tests.ps1 Outdated

		$testCases1 = @(
		@{ Command = {$result = [Collections.Generic.List[int]]@(1)}; CollectionType = 'List`1'; ElementType = "Int32"; Elements = @(1) }
		@{ Command = {$result = [Collections.Generic.List[int]]@(1,2)}; CollectionType = 'List`1'; ElementType = "Int32"; Elements = @(1,2) }
		@{ Command = {$result = [Collections.Generic.List[int]]"4"}; CollectionType = 'List`1'; ElementType = "Int32"; Elements = @(4) }

Jason Shirk (lzybkr) Feb 22, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also test > 1 string converting to List[int]?

Sorry, something went wrong.

Uh oh!

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

All reactions

Dongbo Wang (daxian-dbw) Feb 22, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Fixed.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

test/powershell/Language/Parser/Conversions.Tests.ps1

		param($Command, $CollectionType, $ElementType, $Elements)

		$result = $null
		. $Command

Jason Shirk (lzybkr) Feb 22, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be less surprising to me if Command was just a script block and here you wrote:

$result = . $Command

Sorry, something went wrong.

Uh oh!

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

All reactions

Dongbo Wang (daxian-dbw) Feb 22, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The resulted collection will be unraveled if I use $result = . {[Collections.Generic.List[int]]@(1)}.

In order to prevent unraveling, a comma needs to be prefixed like "{,[Collections.Generic.List[int]]@(1)}".
But then the test case title becomes ",[Collections.Generic.List[int]]@(1)" which I thought was more confusing than "$result = [Collections.Generic.List[int]]@(1)" and that's why I choose the current form.
Do you want me to change back to {, [Collections.Generic.List[int]]@(1)}?

Sorry, something went wrong.

Uh oh!

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

All reactions

Jason Shirk (lzybkr) Feb 22, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see - neither is great - I'm fine either way then.
Because you tried what I suggested first - maybe it's worth a comment to save someone else the trouble trying to change it.

Sorry, something went wrong.

Uh oh!

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

All reactions

Dongbo Wang (daxian-dbw) Feb 22, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Will add a comment.

Sorry, something went wrong.

Uh oh!

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

All reactions

Dongbo Wang (daxian-dbw) Feb 22, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment added.

Sorry, something went wrong.

Uh oh!

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

All reactions

Dongbo Wang (daxian-dbw) added 2 commits

February 22, 2017 13:28


          add test for casting array with more than 1 string elements

eed79d1


          Add a comment

d058513

Jason Shirk (lzybkr) approved these changes

Feb 23, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) merged commit 16ff197 into PowerShell:master

Feb 23, 2017

Dongbo Wang (daxian-dbw) deleted the cast branch

February 23, 2017 07:10

Ilya (iSazonov) removed the Review - Needed The PR is being reviewed label

Mar 27, 2017

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


          Fix casting single element array to generic collection (PowerShell#3170)

46c7c84

Reviewers

Ilya (iSazonov)

iSazonov left review comments

+1 more reviewer

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

Jason Shirk (lzybkr)

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

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.

Conversation

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) Feb 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) commented Feb 22, 2017

Uh oh!

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

Uh oh!

Ilya (iSazonov) commented Feb 22, 2017

Uh oh!

Jason Shirk (lzybkr) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Dongbo Wang (daxian-dbw) Feb 20, 2017 •

edited

Loading

Uh oh!

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