Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Wrapping a call to Registry so that Invoke-WebRequest does not fail on non-text responses#2811

Merged
mirichmo merged 3 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
JonathanMalek:masterJonathanMalek/PowerShell:masterCopy head branch name to clipboard
Dec 2, 2016
Merged

Wrapping a call to Registry so that Invoke-WebRequest does not fail on non-text responses#2811
mirichmo merged 3 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
JonathanMalek:masterJonathanMalek/PowerShell:masterCopy head branch name to clipboard

Conversation

@JonathanMalek

Copy link
Copy Markdown
Contributor

A small bug where content types from an Invoke-WebRequest that are not text-based fall through to a registry check. Wrapping that check (which is additive) with a #if !CORECLR. I can't find any tests that test out that specific behavior, or I would be happy to extend them. I'm also happy to add a new test, just not sure if that is desired.

@msftclas

Copy link
Copy Markdown

Hi @JonathanMalek, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

TTYL, MSBOT;

@PowerShellTeam PowerShellTeam added the Review - Needed The PR is being reviewed label Nov 30, 2016

@mirichmo mirichmo left a comment

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.

In general, we like/need/require tests for all code changes as a forcing function to increase test coverage. In this case, we have internal tests for Invoke-WebRequest that haven't yet been ported to run out of GitHub and you don't have access to them. The tests look like they cover this scenario, so requiring tests from you would be a needless duplication of effort.

|| CheckIsXml(contentType)
|| CheckIsJson(contentType);

#if !CORECLR // Registry access not supported on CoreCLR

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.

PowerShell Core on Windows uses the registry. Use Platform.IsWindows to make it a runtime check.

@mirichmo mirichmo self-assigned this Nov 30, 2016
@mirichmo mirichmo added Review-Waiting on Author and removed Review - Needed The PR is being reviewed labels Nov 30, 2016
@JonathanMalek

Copy link
Copy Markdown
Contributor Author

Thanks for the background @mirichmo. Changed to check for Platform.IsWindows instead.

@PowerShellTeam PowerShellTeam added Review - Needed The PR is being reviewed and removed Review - Waiting on Author labels Dec 1, 2016
@mirichmo mirichmo merged commit 77c622e into PowerShell:master Dec 2, 2016
@mirichmo

mirichmo commented Dec 2, 2016

Copy link
Copy Markdown
Member

@JonathanMalek Thanks for your contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Review - Needed The PR is being reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.