ok2c commented on a change in pull request #333:
URL:
https://github.com/apache/httpcomponents-core/pull/333#discussion_r753662822
##########
File path: httpcore5/src/test/java/org/apache/hc/core5/http/TestHttpHost.java
##########
@@ -69,6 +73,27 @@ public void testConstructor() {
Assert.assertThrows(NullPointerException.class, () -> new
HttpHost(null, (String) null, -1));
Assert.assertThrows(IllegalArgumentException.class, () -> new
HttpHost(null, " ", -1));
Assert.assertThrows(NullPointerException.class, () -> new
HttpHost(null, (InetAddress) null, -1));
+
+ final Host host = new Host("myHost", 8080);
Review comment:
@arturobernalg Please add a test case with non-default scheme such as
`https`. I suspect there will be an unpleasant surprise.
##########
File path: httpcore5/src/main/java/org/apache/hc/core5/http/HttpHost.java
##########
@@ -255,6 +255,22 @@ public HttpHost(final URIAuthority authority) {
this(null, authority);
}
+ /**
+ * Creates {@code HttpHost} instance with the default scheme and the given
{@link
+ * NamedEndpoint}
+ *
+ * @throws IllegalArgumentException If the port parameter is outside the
specified range of
+ * valid port values, which is between 0
and 65535, inclusive.
+ * {@code -1} indicates the scheme
default port.
+ * @see Host
+ * @see URIAuthority
+ * @see org.apache.hc.core5.reactor.EndpointParameters
Review comment:
@arturobernalg This looks wrong
##########
File path: httpcore5/src/test/java/org/apache/hc/core5/http/TestHttpHost.java
##########
@@ -69,6 +73,27 @@ public void testConstructor() {
Assert.assertThrows(NullPointerException.class, () -> new
HttpHost(null, (String) null, -1));
Assert.assertThrows(IllegalArgumentException.class, () -> new
HttpHost(null, " ", -1));
Assert.assertThrows(NullPointerException.class, () -> new
HttpHost(null, (InetAddress) null, -1));
+
+ final Host host = new Host("myHost", 8080);
Review comment:
> I know. but it the same whit the Authority constructor
@arturobernalg It is not the same. The URI authority does not have a scheme
component.
> What you suggest that we should do?
I suggest that we do nothing. Not only this constructor is not very useful,
it can lead to incorrect / unpredictable results.
##########
File path: httpcore5/src/test/java/org/apache/hc/core5/http/TestHttpHost.java
##########
@@ -69,6 +73,27 @@ public void testConstructor() {
Assert.assertThrows(NullPointerException.class, () -> new
HttpHost(null, (String) null, -1));
Assert.assertThrows(IllegalArgumentException.class, () -> new
HttpHost(null, " ", -1));
Assert.assertThrows(NullPointerException.class, () -> new
HttpHost(null, (InetAddress) null, -1));
+
+ final Host host = new Host("myHost", 8080);
Review comment:
No, not nothing. I am going to deprecate `public HttpHost(final
URIAuthority authority)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
