Skip to content

Conversation

@jozkee
Copy link
Member

@jozkee jozkee commented Jul 9, 2019

HttpClient:

  • DefaultProxy
  • DefaultRequestVersion

HttpResponseMessage:

@jozkee jozkee requested a review from karelz as a code owner July 9, 2019 16:54
@jozkee
Copy link
Member Author

jozkee commented Jul 9, 2019

@mairaw @rpetrusha

@jozkee
Copy link
Member Author

jozkee commented Jul 9, 2019

Copy link
Contributor

@scalablecory scalablecory left a comment

Choose a reason for hiding this comment

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

Lots of what looks like invalid xml escaping to correct.

<format type="text/markdown"><![CDATA[
## Remarks
The <xref:System.Net.Http.HttpClient.DefaultProxy%2A> property gets or sets the global proxy. The <xref:System.Net.Http.HttpClient.DefaultProxy%2A> property determines the default proxy that all <xref:System.Net.Http.HttpClient> instances use if no proxy is set explicitly in the <xref:System.Net.Http.HttpClientHandler> passed through its constructor.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the %2A here and elsewhere correct? You may have meant

<xref:System.Net.Http.HttpClient.DefaultProxy/>

Copy link
Contributor

@carlossanlop carlossanlop Jul 15, 2019

Choose a reason for hiding this comment

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

%2A is a star character, not a forward-slash. See here. Not sure if it was intended or not, so it might be a good idea for @jozkee to double check.

In markdown code, the xref elements should not be closed with a forward slash. So the original text is correct. No need to close with forward-slash.

Copy link
Member Author

Choose a reason for hiding this comment

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

I saw the %2A on other properties/methods, I though it was needed when you were not referencing a Type (class/interface/etc.), but it seems is not, therefore I will proceed to remove those %2As. In fact I will remove the entire xref in order to avoid self-reference links.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would still like @mairaw and @rpetrusha to chime in: Is the %2A character allowed in markdown xrefs?

## Remarks
The <xref:System.Net.Http.HttpClient.DefaultProxy%2A> property gets or sets the global proxy. The <xref:System.Net.Http.HttpClient.DefaultProxy%2A> property determines the default proxy that all <xref:System.Net.Http.HttpClient> instances use if no proxy is set explicitly in the <xref:System.Net.Http.HttpClientHandler> passed through its constructor.
If the <xref:System.Net.Http.HttpClient.DefaultProxy%2A> property is overridden, all subsequent instances of the <xref:System.Net.Http.HttpClient> class that does not define its own Proxy in <xref:System.Net.Http.HttpClientHandler> will be using the new defined Proxy in <xref:System.Net.Http.HttpClient.DefaultProxy%2A>.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this paragraph is redundant with the previous one. If you keep it, there is some rewording that needs to be done.

Choose a reason for hiding this comment

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

Since this deals with overrides by a derived class, it's not redundant. But is this actually accurate? You wouldn't expect that a derived class would override not only its behavior, but also the behavior of its base class.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was not trying to talk about class overriding. I meant changing the value of the static property. I will reword that to make it more clear.

If the <xref:System.Net.Http.HttpClient.DefaultProxy%2A> property is overridden, all subsequent instances of the <xref:System.Net.Http.HttpClient> class that does not define its own Proxy in <xref:System.Net.Http.HttpClientHandler> will be using the new defined Proxy in <xref:System.Net.Http.HttpClient.DefaultProxy%2A>.
The <xref:System.Net.Http.HttpClient.DefaultProxy%2A> property reads proxy settings from your system configuration and follows a certain behavior depending on your platform:
Copy link
Contributor

@scalablecory scalablecory Jul 11, 2019

Choose a reason for hiding this comment

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

I don't think we need to document platform-specific behavior here. Can we generalize?

Suggested change
The <xref:System.Net.Http.HttpClient.DefaultProxy%2A> property reads proxy settings from your system configuration and follows a certain behavior depending on your platform:
The default instance returned by the <xref:System.Net.Http.HttpClient.DefaultProxy%2A> property will be configured using environment variables, and if those are not defined, the user's proxy settings.

Copy link
Contributor

Choose a reason for hiding this comment

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

@scalablecory can you elaborate? The result is slightly different depending on the platform, so I think it's great to have those differences explained.

Copy link
Contributor

Choose a reason for hiding this comment

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

@carlossanlop it looks like the platform-specific behavior is simply where the system-wide proxy is configured. I don't know if that's worth documenting here.

However, on second review, if Linux does not have a standard system proxy configuration and so we simply don't try, that specifically seems worth documenting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, agreed. I think it's better to have this documented so we make life easier for Unix users.
No one likes going into the rabbit hole in StackOverflow, wondering why something is not working and not finding any answers.😄

Copy link
Member Author

Choose a reason for hiding this comment

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

I reworded this a little bit on my last commit.

<value>To be added.</value>
<remarks>To be added.</remarks>
</Docs>
<summary>Gets or sets the default version used on subsequent requests made by this <see cref="T:System.Net.Http.HttpClient" /> instance.</summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<summary>Gets or sets the default version used on subsequent requests made by this <see cref="T:System.Net.Http.HttpClient" /> instance.</summary>
<summary>Gets or sets the default HTTP version used on subsequent requests made by this <see cref="T:System.Net.Http.HttpClient" /> instance.</summary>

<remarks>To be added.</remarks>
</Docs>
<summary>Gets or sets the default version used on subsequent requests made by this <see cref="T:System.Net.Http.HttpClient" /> instance.</summary>
<value>The <see cref="P:System.Net.Http.HttpRequestMessage.Version" /> which is going to be set on <see cref="T:System.Net.Http.HttpRequestMessage" /> instances created by this <see cref="T:System.Net.Http.HttpClient" /> instance.</value>
Copy link
Contributor

@scalablecory scalablecory Jul 11, 2019

Choose a reason for hiding this comment

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

Suggested change
<value>The <see cref="P:System.Net.Http.HttpRequestMessage.Version" /> which is going to be set on <see cref="T:System.Net.Http.HttpRequestMessage" /> instances created by this <see cref="T:System.Net.Http.HttpClient" /> instance.</value>
<value>The default version to use for any requests made with this <see cref="T:System.Net.Http.HttpClient" /> instance.</value>

## Remarks
<xref:System.Net.Http.HttpClient.DefaultRequestVersion%2A> is <xref:System.Net.HttpVersion.Version11> by default.
The <xref:System.Net.Http.HttpClient.DefaultRequestVersion%2A> property would be set as the <xref:System.Net.Http.HttpRequestMessage.Version%2A> on any <xref:System.Net.Http.HttpRequestMessage> the <xref:System.Net.Http.HttpClient> instance creates, in example, as part of a call to <xref:System.Net.Http.HttpClient.GetAsync%2A>, <xref:System.Net.Http.HttpClient.GetStringAsync%2A>, <xref:System.Net.Http.HttpClient.GetByteArrayAsync%2A>, etc.
Copy link
Contributor

@scalablecory scalablecory Jul 11, 2019

Choose a reason for hiding this comment

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

Suggested change
The <xref:System.Net.Http.HttpClient.DefaultRequestVersion%2A> property would be set as the <xref:System.Net.Http.HttpRequestMessage.Version%2A> on any <xref:System.Net.Http.HttpRequestMessage> the <xref:System.Net.Http.HttpClient> instance creates, in example, as part of a call to <xref:System.Net.Http.HttpClient.GetAsync%2A>, <xref:System.Net.Http.HttpClient.GetStringAsync%2A>, <xref:System.Net.Http.HttpClient.GetByteArrayAsync%2A>, etc.
The `DefaultRequestVersion` property specifies the default HTTP version to use for any requests sent using this <xref:System.Net.Http.HttpClient> instance, such as calls to <xref:System.Net.Http.HttpClient.GetAsync%2A>, <xref:System.Net.Http.HttpClient.GetStringAsync%2A>, or <xref:System.Net.Http.HttpClient.SendAsync%2A>.

@carlossanlop carlossanlop self-requested a review July 15, 2019 18:21
Copy link

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

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

I've left a few comments and some suggested changes, @jozkee. Also, using an xref to represent the current member currently creates a self-referencing link, which causes a page reload when the user selects it. They're best fenced to present this.

<value>To be added.</value>
<remarks>To be added.</remarks>
<summary>Gets or sets the global Http proxy.</summary>
<value>A proxy used by every call that instantiates a <see cref="T:System.Net.Http.HttpWebRequest" />.</value>

Choose a reason for hiding this comment

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

Suggested change
<value>A proxy used by every call that instantiates a <see cref="T:System.Net.Http.HttpWebRequest" />.</value>
<value>A proxy used by every call that instantiates a <see cref="T:System.Net.HttpWebRequest" />.</value>

This fixes the one broken xref reported by the build.

<format type="text/markdown"><![CDATA[
## Remarks
The <xref:System.Net.Http.HttpClient.DefaultProxy%2A> property gets or sets the global proxy. The <xref:System.Net.Http.HttpClient.DefaultProxy%2A> property determines the default proxy that all <xref:System.Net.Http.HttpClient> instances use if no proxy is set explicitly in the <xref:System.Net.Http.HttpClientHandler> passed through its constructor.

Choose a reason for hiding this comment

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

Suggested change
The <xref:System.Net.Http.HttpClient.DefaultProxy%2A> property gets or sets the global proxy. The <xref:System.Net.Http.HttpClient.DefaultProxy%2A> property determines the default proxy that all <xref:System.Net.Http.HttpClient> instances use if no proxy is set explicitly in the <xref:System.Net.Http.HttpClientHandler> passed through its constructor.
This static property determines the default proxy that all <xref:System.Net.Http.HttpClient> instances use if no proxy is set explicitly in the <xref:System.Net.Http.HttpClientHandler> passed through its constructor.

## Remarks
The <xref:System.Net.Http.HttpClient.DefaultProxy%2A> property gets or sets the global proxy. The <xref:System.Net.Http.HttpClient.DefaultProxy%2A> property determines the default proxy that all <xref:System.Net.Http.HttpClient> instances use if no proxy is set explicitly in the <xref:System.Net.Http.HttpClientHandler> passed through its constructor.
If the <xref:System.Net.Http.HttpClient.DefaultProxy%2A> property is overridden, all subsequent instances of the <xref:System.Net.Http.HttpClient> class that does not define its own Proxy in <xref:System.Net.Http.HttpClientHandler> will be using the new defined Proxy in <xref:System.Net.Http.HttpClient.DefaultProxy%2A>.

Choose a reason for hiding this comment

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

Since this deals with overrides by a derived class, it's not redundant. But is this actually accurate? You wouldn't expect that a derived class would override not only its behavior, but also the behavior of its base class.

* **For Linux:** Reads proxy configuration from Environment Variables, If they are not defined, no proxy is defined.
The Environment Variables used for the DefaultProxy initialization are:

Choose a reason for hiding this comment

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

An explanation of what each environment variable does would be useful.

Suggested change
The Environment Variables used for the DefaultProxy initialization are:
The environment variables used for `DefaultProxy` initialization on Windows and Unix-based platforms are:

* ALL_PROXY
* NO_PROXY
The name of Environment Variables remain constant across Windows, OSX, and Linux platforms.

Choose a reason for hiding this comment

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

Suggested change
The name of Environment Variables remain constant across Windows, OSX, and Linux platforms.

With the previous change, this sentence can be deleted.

The <xref:System.Net.Http.HttpClient.DefaultRequestVersion%2A> property can be overriden as long as the <xref:System.Net.Http.HttpClient> instance has not started any request.
]]></format>
</remarks>
<exception cref="T:System.ArgumentNullException">Cannot set a <see langword="null" /> value to <see cref="P:System.Net.Http.HttpClient.DefaultRequestVersion" />.</exception>

Choose a reason for hiding this comment

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

Suggested change
<exception cref="T:System.ArgumentNullException">Cannot set a <see langword="null" /> value to <see cref="P:System.Net.Http.HttpClient.DefaultRequestVersion" />.</exception>
<exception cref="T:System.ArgumentNullException">In a set operation, <see langword="DefaultRequestVersion" /> is <see langword="null" />.</exception>

</remarks>
<exception cref="T:System.ArgumentNullException">Cannot set a <see langword="null" /> value to <see cref="P:System.Net.Http.HttpClient.DefaultRequestVersion" />.</exception>
<exception cref="T:System.InvalidOperationException">The <see cref="T:System.Net.Http.HttpClient" /> instance has already started one or more requests.</exception>
<exception cref="T:System.ObjectDisposedException"><see cref="T:System.Net.Http.HttpClient" /> instance has already been disposed.</exception>

Choose a reason for hiding this comment

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

Suggested change
<exception cref="T:System.ObjectDisposedException"><see cref="T:System.Net.Http.HttpClient" /> instance has already been disposed.</exception>
<exception cref="T:System.ObjectDisposedException">The <see cref="T:System.Net.Http.HttpClient" /> instance has already been disposed.</exception>

<summary>To be added.</summary>
<value>To be added.</value>
<remarks>To be added.</remarks>
<summary>Gets the collection of trailing headers included in a HTTP response.</summary>

Choose a reason for hiding this comment

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

Suggested change
<summary>Gets the collection of trailing headers included in a HTTP response.</summary>
<summary>Gets the collection of trailing headers included in an HTTP response.</summary>

## Remarks
Forbidden headers will be ignored.
Since trailing headers are send as a HTTP header appended at the end of the response message; the <xref:System.Net.Http.HttpResponseMessage.TrailingHeaders%2A> property will return an empty HttpResponseHeaders instance if its accessed and the response content has not been read completely (See example below).

Choose a reason for hiding this comment

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

Suggested change
Since trailing headers are send as a HTTP header appended at the end of the response message; the <xref:System.Net.Http.HttpResponseMessage.TrailingHeaders%2A> property will return an empty HttpResponseHeaders instance if its accessed and the response content has not been read completely (See example below).
Since trailing headers are send as an HTTP header appended at the end of the response message, the `TrailingHeaders` property returns an empty <xref:System.Net.Http.Headers.HttpResponseHeaders> instance if it is accessed and the response content has not been read completely, as the following example shows.

Were you planning to include an examples section for the example (in which case it won't follow) or to include in the Remarks after this paragraph (in which case it does follow)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was planning to include it as a code snippet (under #2734) right after this paragraph. So it does follow I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will remove as the following example shows and I will add it back on #2734.

Since trailing headers are send as a HTTP header appended at the end of the response message; the <xref:System.Net.Http.HttpResponseMessage.TrailingHeaders%2A> property will return an empty HttpResponseHeaders instance if its accessed and the response content has not been read completely (See example below).
For further information on trailing headers see [RFC 7230 - 4.1.2. Chunked Trailer Part](https://tools.ietf.org/html/rfc7230#section-4.1.2).

Choose a reason for hiding this comment

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

Suggested change
For further information on trailing headers see [RFC 7230 - 4.1.2. Chunked Trailer Part](https://tools.ietf.org/html/rfc7230#section-4.1.2).
For additional information on trailing headers, see [RFC 7230 - 4.1.2. Chunked Trailer Part](https://tools.ietf.org/html/rfc7230#section-4.1.2).

@carlossanlop carlossanlop added 🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 new-content Indicates PRs that contain new articles labels Jul 19, 2019
Reworded  DefaultProxy platform-specific configuration.
@jozkee
Copy link
Member Author

jozkee commented Jul 23, 2019

Ready for re-review @rpetrusha @mairaw @scalablecory.

@mairaw mairaw added this to the July 2019 milestone Jul 24, 2019
Copy link

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

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

Thanks for addressing all of the comments, @jozkee. I just have a few additional suggestions and this will be ready to merge. You can accept my suggestions in a single commit by reviewing them from the Files changed tab.

The default instance returned by this property will initialize following a different set of rules depending on your platform:
* **For Windows:** Reads proxy configuration from environment variables, if those are not defined, the user's proxy settings.
* **For OSX:** Reads proxy configuration from environment variables, if those are not defined, the system's proxy settings.

Choose a reason for hiding this comment

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

Suggested change
* **For OSX:** Reads proxy configuration from environment variables, if those are not defined, the system's proxy settings.
* **For OSX:** Reads proxy configuration from environment variables or, if those are not defined, from the system's proxy settings.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will add a small change. please see https://github.com/dotnet/dotnet-api-docs/pull/2822/files#r307027943
Replacing OSX for macOS.

Suggested change
* **For OSX:** Reads proxy configuration from environment variables, if those are not defined, the system's proxy settings.
* **For macOS:** Reads proxy configuration from environment variables or, if those are not defined, from the system's proxy settings.

* **For OSX:** Reads proxy configuration from environment variables, if those are not defined, the system's proxy settings.
* **For Unix:** Reads proxy configuration from environment variables, in case those are not defined, this property initializes a non-configured instance that bypasses all addresses.

Choose a reason for hiding this comment

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

Suggested change
* **For Unix:** Reads proxy configuration from environment variables, in case those are not defined, this property initializes a non-configured instance that bypasses all addresses.
* **For Unix:** Reads proxy configuration from environment variables or, in case those are not defined, this property initializes a non-configured instance that bypasses all addresses.

Copy link
Member Author

Choose a reason for hiding this comment

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

Replacing Unix for Linux, see https://github.com/dotnet/dotnet-api-docs/pull/2822/files#r307043538

Suggested change
* **For Unix:** Reads proxy configuration from environment variables, in case those are not defined, this property initializes a non-configured instance that bypasses all addresses.
* **For Linux:** Reads proxy configuration from environment variables or, in case those are not defined, this property initializes a non-configured instance that bypasses all addresses.

* Applying lexical suggestions.
* replaced OSX for macOS and Unix for Linux.

Co-Authored-By: Ron Petrusha <[email protected]>
Copy link

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

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

Thanks for the additional set of changes, @jozkee. I'll merge your PR now.

@rpetrusha rpetrusha merged commit d20148e into dotnet:master Jul 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.Net 🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 new-content Indicates PRs that contain new articles

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants