Skip to content

Document WebResponseObject/BasicHtmlWebResponseObject properties #11876

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
adityapatwardhan merged 2 commits into from
Mar 23, 2020
Merged

Document WebResponseObject/BasicHtmlWebResponseObject properties #11876

adityapatwardhan merged 2 commits into from
Mar 23, 2020

Conversation

@kevinoid
Copy link
Contributor

@kevinoid kevinoid commented Feb 18, 2020

PR Summary

Improve xmldoc comments for properties of WebResponseObject and BasicHtmlWebResponseObject.

PR Context

I found the current documentation to be unhelpful for understanding the values returned by Invoke-WebRequest. This PR contains my current understanding after examining the code. Feel free to modify or rewrite to better fit your preferred style.

PR Checklist

@kevinoid
Document {BasicHtml,}WebResponseObject properties
34b7fd2 
The properties of WebResponseObject and BasicHtmlWebResponseObject had
xmldoc comments which were not particularly informative.  Add more
informative comments to help users better understand the values returned
by these properties.

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
@adityapatwardhan
Copy link
Member

adityapatwardhan commented Feb 18, 2020

@PoshChan please rebuild all

@PoshChan
Copy link
Collaborator

PoshChan commented Feb 18, 2020

@adityapatwardhan, successfully started rebuild of PowerShell-CI-static-analysis, PowerShell-CI-Windows, PowerShell-CI-macOS, PowerShell-CI-Linux

@kevinoid
Copy link
Contributor Author

kevinoid commented Feb 19, 2020 •
edited
Loading

I should also mention that .Content and .RawContentStream contain only the response content, while .RawContent contains status, headers, and content. I tried to make that clear in the comments, but I suspect it will still catch users by surprise. Assuming this is intentional and none of the properties will be deprecated, would bold be appropriate to draw attention to this difference?

@kevinoid
Copy link
Contributor Author

kevinoid commented Feb 19, 2020

Also, since BasicHtmlWebResponseObject doesn't override .ToString(), it will differ from .Content and .RawContent since it contains the response content decoded as ASCII with non-printable characters removed. Although it meets the definition of a "string representation", which is all that it is documented to return, I worry it may also catch users by surprise if they write code which uses ToString().

@iSazonov iSazonov requested a review from sdwheeler February 19, 2020 04:53
@iSazonov iSazonov added the CL-Docs Indicates that a PR should be marked as a documentation change in the Change Log label Feb 19, 2020
@iSazonov iSazonov added this to the 7.1.0-preview.1 milestone Feb 19, 2020
@kevinoid @iSazonov
Apply suggestions from @iSazonov
e3be623 
Co-Authored-By: Ilya <darpa@yandex.ru>
@adityapatwardhan adityapatwardhan merged commit 68442a3 into PowerShell:master Mar 23, 2020
@adityapatwardhan
Copy link
Member

adityapatwardhan commented Mar 23, 2020

@kevinoid Thank you for your contribution!

@kevinoid
Copy link
Contributor Author

kevinoid commented Mar 23, 2020

My pleasure. Thanks for reviewing and merging @adityapatwardhan!

@ghost
Copy link

ghost commented Apr 23, 2020

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

Handy links:

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

Reviewers

@iSazonov iSazonov iSazonov left review comments

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

@sdwheeler sdwheeler Awaiting requested review from sdwheeler

Assignees

@adityapatwardhan adityapatwardhan

Labels

CL-Docs Indicates that a PR should be marked as a documentation change in the Change Log

Projects

None yet

Milestone

7.1.0-preview.2

Development

Successfully merging this pull request may close these issues.

4 participants

@kevinoid @adityapatwardhan @PoshChan @iSazonov