-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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.
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
Add HttpVersion parameter to Invoke-WebRequest cmdlet #15853
Add HttpVersion parameter to Invoke-WebRequest cmdlet #15853
Conversation
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.
As for test, I think we have no need to do real HTTP requests with specified version since we utilize .Net Runtime features. I think it would enough if you did manual checks and shared results.
Nevertheless, we must verify that (1) the parameter accepts well-know values without throw, (2) transformation attribute works (it could be xUnit tests), (3) tab-completion works.
...rosoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/HttpVersionCompleter.cs
Outdated
Show resolved
Hide resolved
...rosoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/HttpVersionCompleter.cs
Outdated
Show resolved
Hide resolved
...ell.Commands.Utility/commands/utility/WebCmdlet/Common/HttpVersionTransformationAttribute.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/HttpVersionUtils.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.Commands.Utility/resources/WebCmdletStrings.resx
Outdated
Show resolved
Hide resolved
...ell.Commands.Utility/commands/utility/WebCmdlet/Common/HttpVersionTransformationAttribute.cs
Outdated
Show resolved
Hide resolved
|
...ell.Commands.Utility/commands/utility/WebCmdlet/Common/HttpVersionTransformationAttribute.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/HttpVersionUtils.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/HttpVersionUtils.cs
Outdated
Show resolved
Hide resolved
f2c9643
to
5cd00b7
Compare
src/Microsoft.PowerShell.Commands.Utility/resources/WebCmdletStrings.resx
Show resolved
Hide resolved
...rosoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/HttpVersionCompleter.cs
Outdated
Show resolved
Hide resolved
6fd9a05
to
cb4cb12
Compare
4645764
to
7897ad8
Compare
src/System.Management.Automation/engine/ArgumentToVersionTransformationAttribute.cs
Outdated
Show resolved
Hide resolved
7897ad8
to
753ab24
Compare
I have no idea about failed tests ) |
d3a3a78
to
84e6695
Compare
@hayhay27 Please update the PR description to address latest code changes. |
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.
Please update the PR description to address latest code changes.
done
while we keep silence I thought with your blessing I can update docs for this PR |
add tests cases make ArgumentToVersionTransformationAttribute internal
…Cmdlet/Common/HttpVersionUtils.cs
add NRE handling
add pester tests
skip tests for HttpVersion on mac and linux
fix code smells
04bcb4f
to
83ce173
Compare
fixes by review - remove HttpVersionUtils - refactor pester test
@rjmholt Thanks a lot! |
@rjmholt Can you please take another look? |
...rShell.Commands.Utility/commands/utility/WebCmdlet/Common/HttpVersionCompletionsAttribute.cs
Outdated
Show resolved
Hide resolved
...rShell.Commands.Utility/commands/utility/WebCmdlet/Common/HttpVersionCompletionsAttribute.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/ArgumentToVersionTransformationAttribute.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/ArgumentToVersionTransformationAttribute.cs
Outdated
Show resolved
Hide resolved
fixes by review - extend version transformation via method override instead of ctor injection - clean up code
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
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.
LGTM
@hayhay27 Thank you for your contribution! |
🎉 Handy links: |
PR Summary
Fix #12641
HttpVersionCompletionsAttribute
with completions1.0
,1.1
,2.0
,3.0
string
,int
anddouble
toVersion
(generalizeArgumentToVersionTransformationAttribute
fromSet-StrictMode
)HttpVersion
completionsArgumentToVersionTransformationAttribute
PR Context
This PR addressed to issue #12641
At the moment impossible make requests to network resources served only with HTTP/2 cause of default http version in
HttpRequestMessage.Version
=1.1Changes in this PR allow to set http version during invocation
Invoke-RestMethod
orInvoke-WebRequest
This is my first PR to PowerShell. I have a couple of questions about tests. How and where to implement (at least point me right place please).
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).