-
Notifications
You must be signed in to change notification settings - Fork 7.3k
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
Improve Invoke-WebRequest xml and json errors format #18837
Improve Invoke-WebRequest xml and json errors format #18837
Conversation
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
Formally LGTM. It would great to see output examples for all cases (XML, HTML, Json and Text). |
I'm completely satisfied with XML errors *example powershell regex as a starting point ($jsonerror -replace "\s*{|\s*}|\s*\[|\s*\]|`"|`,","" -split"`n")|?{$_} output
|
It's just an error message so I wouldn't waste too much effort on it. I personally would not remove HTML tags, but since this has already been given preference, let it remain so. I'd say improving the readability of the error message is good, but we're not preserving the original information, which surprises me. |
@iSazonov We could stop removing HTML tags (it generates ugly errors with far too much white space in many cases); and format XML without removing tags. |
Yes, it looks ugly. I don't know why this was done. If there is simple way (like for XML or JSON) to re-format HTML you could add this too. |
Unfortunately I dont think there is an easy way to re-format HTML without using a third party library (ex. AngleSharp). |
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 with latest screenshots.
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
@CarloToso Please clarify what the helper service returns and how tests work now. If it returns non-json how we check json scenario? |
@iSazonov The helper service now returns:
|
@CarloToso Please update the PR description and update to actual screenshots. |
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
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) |
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Show resolved
Hide resolved
🎉 Handy links: |
PR Summary
Currently Invoke-WebRequest completely mangles XML error responses:
After this PR it recognizes XML and Json and throws better errors:
No changes for text and html errors.
PR Context
Fixes #13251