KEMBAR78
Make `-OutFile` param in web cmdlets to work like -LiteralPath by iSazonov · Pull Request #11701 · PowerShell/PowerShell · GitHub
Skip to content

Make -OutFile param in web cmdlets to work like -LiteralPath #11701

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 6 commits into from
May 20, 2020
Merged

Make -OutFile param in web cmdlets to work like -LiteralPath #11701

merged 6 commits into from
May 20, 2020

Conversation

@iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented Jan 28, 2020 •
edited
Loading

PR Summary

Now OutFile parameter in Invoke-WebRequest and Invoke-RestMethod works like LiteralPath and does not process wildcards.

PR Context

Fix #3174 - approved by PowerShell Committee.

PR Checklist

@iSazonov iSazonov added Documentation Needed in this repo Documentation is needed in this repo CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log labels Jan 28, 2020
@iSazonov iSazonov added this to the 7.1.0-preview.1 milestone Jan 28, 2020
@ghost ghost assigned TravisEz13 Jan 28, 2020
@iSazonov iSazonov force-pushed the webcmdlet-outfile-literal branch from 6d80ba9 to 75de300 Compare January 28, 2020 12:05
@rkeithhill
Copy link
Collaborator

I don't particularly like that the parameter name doesn't indicate the direction - out. What if in the future, we want to implement file uploading (like curl's -F). Then you are kind of boxed into a corner here because you've already used the parameters you typically would use for source paths (Path and LiteralPath).

@iSazonov
Copy link
Collaborator Author

I don't particularly like that the parameter name doesn't indicate the direction - out. What if in the future, we want to implement file uploading (like curl's -F)

The change was approved as a compromise because we have RFC for web file operations cmdlet.

/cc @mklement0

@rkeithhill
Copy link
Collaborator

Irrespective of the parameter "future-proofing" argument, if I'm looking at code and I see Invoke-WebRequest -Path C:\foo.txt -Uri http://foo.com/foo.txt, it isn't clear that this is a download versus an upload.

@mklement0
Copy link
Contributor

mklement0 commented Jan 29, 2020 •
edited
Loading

I agree that retaining the Out prefix is desirable for conceptual clarity (even if Invoke-WebRequest / Invoke-RestMethod themselves will likely never see upload parameters, due to the plan to cover upload/download functionality in separate cmdlets, as proposed in PowerShell/PowerShell-RFC#124 for downloading, for instance).

Taking a step back:

-OutFile should never have accepted wildcard patterns (just as it wasn't appropriate in various other contexts: #4726, #9225).

The reason for the decision in #3174 was a backward-compatibility concern: conceivably, someone could have used iwr .. -outfile hello* an, if there happened to be exactly one existing file that matches this pattern, iwr would have saved to it.

Is this a real-world concern? It strikes me as a clear-cut bucket 3 case.

Thumbing through GitHub code, I don't see instances of wildcard use, at least at first glance:

Given that the workaround for the inability to use [ and ] as-is in the file name with `[ and `] didn't work either, we probably don't need to worry about breaking workarounds either.

Therefore, we could:

  • simply change -OutFile to accept only literal paths going forward.

  • give it an alias of -OutPath in the context of the green-lighted experimental feature to allow specifying a directory path only and have the output file placed there with the server-specified file name.

@iSazonov
Copy link
Collaborator Author

/cc @SteveL-MSFT For PowerShell Committee - in #3174 a conclusion was #3174 (comment) rename OutFile to Path and add LiteralPath. Current @mklement0's request is review this again to leave OutFile parameter.

@iSazonov iSazonov changed the title Replace OutFile parameter with Path and LiteralPath in web cmdlets WIP: Replace OutFile parameter with Path and LiteralPath in web cmdlets Jan 29, 2020
@SteveL-MSFT SteveL-MSFT added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Jan 29, 2020
JamesWTruher
JamesWTruher reviewed Jan 29, 2020
View reviewed changes
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

all the other cmdlets which provide LiteralPath also have a PSPath alias. Should that be done here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems the PSPath is mainly used for pipeline parameter binding. Because the parameter doesn't get arguments from pipeline I removed the alias.

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you add some commentary why you didn't use parameter sets here? If we ever change our parameter set logic, this could be one of those places where we could take advantage of it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have already 3 parameter sets and class inheritance. I'm afraid adding two more parameter sets highly complicates the code and may turn into a headache. I'd prefer to postpone this to another PR if we really want this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

just to be clear, I wasn't suggesting additional parameter sets. Your reasoning is sound, it's pretty complicated, and doesn't need to be more so. I was just looking for commentary in the code as to why so others doing reviews or later maintenance would understand.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The change was reverted as PowerShell Committee request.

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo:

Suggested change
Context "Path and LigeralPath parameters work" {
Context "Path and LiteralPath parameters work" {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
It "Cannot use both Path and LigeralPath parameters" {
It "Cannot use both Path and LiteralPath parameters" {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
It "Path and LigeralPath parameters work in Invoke-WebRequest" {
It "Path and LiteralPath parameters work in Invoke-WebRequest" {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
It "Path and LigeralPath parameters work in Invoke-RestMethod" {
It "Path and LiteralPath parameters work in Invoke-RestMethod" {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@JamesWTruher
Copy link
Collaborator

@iSazonov is this still WIP? if not, please change the title.

@SteveL-MSFT
Copy link
Member

@PowerShell/powershell-committee reviewed this again. @iSazonov apologies on changing our decision on you, but we agree with the current proposal to have -OutFile simply act as though it was -LiteralPath. We believe this does fit as a bucket 3 item as it's unlikely a user would use a wildcard with this cmdlet to overwrite an existing file where they would use tab completion to get the exact file name.

@SteveL-MSFT SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Feb 26, 2020
@iSazonov iSazonov force-pushed the webcmdlet-outfile-literal branch from af87a08 to 0891da7 Compare February 27, 2020 18:26
@iSazonov iSazonov changed the title WIP: Replace OutFile parameter with Path and LiteralPath in web cmdlets Replace OutFile parameter with Path and LiteralPath in web cmdlets Mar 13, 2020
@iSazonov iSazonov force-pushed the webcmdlet-outfile-literal branch from 0891da7 to bb45d55 Compare March 13, 2020 14:51
@iSazonov
Copy link
Collaborator Author

@SteveL-MSFT @TravisEz13 The PR is ready to review.
CI MacOs is temporary failed.

@mklement0 mklement0 mentioned this pull request Mar 21, 2020
Open
@iSazonov iSazonov added the CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log label Mar 28, 2020
@iSazonov iSazonov mentioned this pull request Mar 28, 2020
Closed
11 tasks
@iSazonov iSazonov force-pushed the webcmdlet-outfile-literal branch from bb45d55 to e0e2d5a Compare March 28, 2020 18:21
@iSazonov
Copy link
Collaborator Author

@TravisEz13 @SteveL-MSFT Please review the PR.

@TravisEz13 TravisEz13 changed the title Make OutFile parameter in web cmdlets to work like LiteralPath Make -OutFile param in web cmdlets to work like -LiteralPath May 20, 2020
@TravisEz13 TravisEz13 merged commit 16c1a36 into PowerShell:master May 20, 2020
@iSazonov iSazonov deleted the webcmdlet-outfile-literal branch May 21, 2020 03:02
@sdwheeler sdwheeler mentioned this pull request Jun 18, 2020
Closed
@ghost
Copy link

ghost commented Jun 25, 2020

🎉v7.1.0-preview.4 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Jun 25, 2020
Closed
@mklement0 mklement0 mentioned this pull request Jul 8, 2020
Closed
@mklement0 mklement0 mentioned this pull request Apr 5, 2022
Closed
5 tasks
@iSazonov iSazonov mentioned this pull request Jul 14, 2022
Merged
13 tasks
@mklement0 mklement0 mentioned this pull request Oct 20, 2023
Closed
5 tasks
@mklement0 mklement0 mentioned this pull request Dec 14, 2023
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TravisEz13 TravisEz13 TravisEz13 approved these changes

@SteveL-MSFT SteveL-MSFT Awaiting requested review from SteveL-MSFT

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

+2 more reviewers

@JamesWTruher JamesWTruher JamesWTruher left review comments

@PoshChan PoshChan PoshChan left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@TravisEz13 TravisEz13

Labels

CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Milestone

7.1.0-preview.4

Development

Successfully merging this pull request may close these issues.

Discussion: Use literal paths for OutFile parameter on Invoke-RestMethod / Invoke-WebRequest

7 participants

@iSazonov @rkeithhill @mklement0 @JamesWTruher @SteveL-MSFT @TravisEz13 @PoshChan