FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Make Move-Item work with its -Include, -Exclude, and -Filter parameters by jeffbi · Pull Request #3878 · PowerShell/PowerShell · GitHub

/ PowerShell Public

Make Move-Item work with its -Include, -Exclude, and -Filter parameters - #3878

Merged
Travis Plunk (TravisEz13) merged 4 commits into
PowerShell:masterfrom
jeffbi:move-item-2385
Jun 23, 2017
ConversationCommits (4)ChecksFiles changed
Merged

Make Move-Item work with its -Include, -Exclude, and -Filter parameters#3878
Travis Plunk (TravisEz13) merged 4 commits into
PowerShell:masterfrom
jeffbi:move-item-2385

Conversation

jeffbi commented May 30, 2017

Copy link
Copy Markdown

Fixed #2385

Invoke the correct overload of SessionState.Path.GetResolvedPSPathFromPSPath, passing the cmdlet context object.

Make Move-Item work with its -Include, -Exclude, and -Filter paramete…
83b2d6e 
…rs (#2385)

Invoke the correct overload of SessionState.Path.GetResolvedPSPathFromPSPath, passing the cmdlet context object.

jeffbi commented May 30, 2017

Copy link
Copy Markdown
Author

Ilya (@iSazonov) Can you take a look at the Appveyor failure? I'm looking at the details and they seem to show two failure points, neither of which have anything to do with this PR.

Ilya (iSazonov) commented May 30, 2017 •
edited
Loading

Copy link
Copy Markdown
Collaborator

jeffbi I see the same in other PRs. Nightly builds is affected too.

Ilya (iSazonov) reviewed May 30, 2017
View reviewed changes

Ilya (iSazonov) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Original Issue say that the cmdlet works well but writes unexpected error message - I don't see the check in tests.

jeffbi commented May 30, 2017 •
edited
Loading

Copy link
Copy Markdown
Author

Ilya (@iSazonov) Well, what it actually said what that the cmdlet successfully performed the action that was intended, then wrote an error. That's not quite the same as it worked well.

I've updated the tests to check that the Move-Item was successful, and also to verify that files meant to be unaffected were unaffected.

Ilya (iSazonov) commented May 30, 2017

Copy link
Copy Markdown
Collaborator

LGTM.

Aditya Patwardhan (adityapatwardhan) requested changes Jun 5, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/Move-Item.Tests.ps1 Outdated
$booContent = "boo content"
}
BeforeEach {
New-Item -ItemType Directory -Path $filterPath

Aditya Patwardhan (adityapatwardhan) Jun 5, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should be

New-Item -ItemType Directory -Path $filterPath | Out-Null

to avoid on console output of created item

jeffbi Jun 5, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed.

Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/Move-Item.Tests.ps1 Outdated
It "Can move to different directory, filtered with -Include" {
Move-Item -Path $filePath -Destination $moveToPath -Include "bar*"
$? | Should Be $true
Test-Path -Path $barPath | Should Be $false

Aditya Patwardhan (adityapatwardhan) Jun 5, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Use Powershell $barPath | Should Not Exist"

jeffbi Jun 5, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed

Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/Move-Item.Tests.ps1 Outdated
}
It "Can move to different directory, filtered with -Include" {
Move-Item -Path $filePath -Destination $moveToPath -Include "bar*"
$? | Should Be $true

Aditya Patwardhan (adityapatwardhan) Jun 5, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Difficult to debug from logs. Instead use

Move-Item -Path $filePath -Destination $moveToPath -Include "bar*" -ErrorVariable e -ErrorAction SilentlyContinue
$e | Should BeNullOrEmpty

jeffbi Jun 5, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed

Ilya (iSazonov) reviewed Jun 6, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/Move-Item.Tests.ps1 Outdated
It "Can move to different directory, filtered with -Include" {
Move-Item -Path $filePath -Destination $moveToPath -Include "bar*" -ErrorVariable e -ErrorAction SilentlyContinue
$e | Should BeNullOrEmpty
#Test-Path -Path $barPath | Should Be $false

Ilya (iSazonov) Jun 6, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please remove the comment.

jeffbi Jun 6, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for catching that. Fixed.

Ilya (iSazonov) commented Jun 6, 2017

Copy link
Copy Markdown
Collaborator

jeffbi Thanks for the fix!

LGTM. (After removing one unneeded comment)

jeffbi commented Jun 6, 2017

Copy link
Copy Markdown
Author

Ilya (@iSazonov), Aditya Patwardhan (@adityapatwardhan) Thanks for the review.

Travis Plunk (TravisEz13) commented Jun 8, 2017

Copy link
Copy Markdown
Member

jeffbi Can you update or remove the company in your profile? If it is accurate, please email me internally about additional steps you need to take.

Aditya Patwardhan (@adityapatwardhan) Please make your Microsoft Organization membership public.

Travis Plunk (TravisEz13) merged commit 5ee4ec1 into PowerShell:master Jun 23, 2017
Mike Richmond (mirichmo) added a commit that referenced this pull request Jun 23, 2017
Revert "Make Move-Item work with its -Include, -Exclude, and -Filter …
76d4888 
…parameters (#3878)"

This reverts commit 5ee4ec1.
jeffbi deleted the move-item-2385 branch July 11, 2017 19:24
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026
Make Move-Item work with its -Include, -Exclude, and -Filter paramete…
6aaf735 
…rs (PowerShell#3878)

* Make Move-Item work with its -Include, -Exclude, and -Filter parameters (PowerShell#2385)

Invoke the correct overload of SessionState.Path.GetResolvedPSPathFromPSPath, passing the cmdlet context object.

* Update tests per code review.

* Changes per code review.

* Remove stray comment
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Ilya (iSazonov) iSazonov left review comments
Aditya Patwardhan (adityapatwardhan) adityapatwardhan approved these changes

Assignees

Travis Plunk (TravisEz13)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL