On Wed, 4 Nov 2020 15:11:50 GMT, Daniel Fuchs <dfu...@openjdk.org> wrote:
>> Patrick Concannon has updated the pull request with a new target base due to >> a merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains six additional >> commits since the last revision: >> >> - 8252304: Removed catch block from newBuilder(HttpRequest) >> - 8252304: assertBodyPublisherEqual added to test; added comment to >> newBuilder >> - Merge remote-tracking branch 'origin/master' into JDK-8252304 >> - Merge remote-tracking branch 'origin/master' into JDK-8252304 >> - Merge remote-tracking branch 'origin/master' into JDK-8252304 >> - 8252304: Seed an HttpRequest.Builder from an existing HttpRequest > > src/java.net.http/share/classes/java/net/http/HttpRequest.java line 317: > >> 315: * the given request (for instance, if the request contains >> illegal >> 316: * parameters) >> 317: * @since TBD > > `@since 16` > > we can change it again if it doesn't make the cut Release number added in commit https://github.com/openjdk/jdk/pull/1059/commits/7928083ae954c44b5357064b86af0e1a3f53588b > src/java.net.http/share/classes/java/net/http/HttpRequest.java line 339: > >> 337: } >> 338: } >> 339: ); > > I know what this piece of code does, and why it is necessary, but it would be > good to add some `//` comments above to explain what is going on for the > benefice of future maintainers Comment added to clarify ifPresentOrElese statment. See commit https://github.com/openjdk/jdk/pull/1059/commits/7928083ae954c44b5357064b86af0e1a3f53588b > test/jdk/java/net/httpclient/HttpRequestNewBuilderTest.java line 272: > >> 270: assertThrows(IAE, () -> HttpRequest.newBuilder(r).build()); >> 271: } >> 272: } > > It is customary to have a new line at the end of files. New line added at end of file. See commit https://github.com/openjdk/jdk/pull/1059/commits/7928083ae954c44b5357064b86af0e1a3f53588b > test/jdk/java/net/httpclient/HttpRequestNewBuilderTest.java line 63: > >> 61: new NamedAssertion("headers", (r1,r2) -> >> assertEquals(r1.headers(), r2.headers())), >> 62: new NamedAssertion("expectContinue", (r1,r2) -> >> assertEquals(r1.expectContinue(), r2.expectContinue())) >> 63: ); > > There should be an assertion for request bodies as well. You could check the > following: > > 1. if r1 has a body, r2 must have a body > 2. if r1 has no body, then r2 must not have a body > 3. if both r1 and r2 have a body, then the body type & content length should > be identical > 4. you could subscribe to each body publisher and verify that the set of > bytes you eventually get are identical Additional assertion added to check all 4 points raised see L109-142 in commit https://github.com/openjdk/jdk/pull/1059/commits/7928083ae954c44b5357064b86af0e1a3f53588b > test/jdk/java/net/httpclient/HttpRequestNewBuilderTest.java line 47: > >> 45: * @test >> 46: * @bug 8252304 >> 47: * @summary HttpRequest.NewBuilder(HttpRequest) API and behaviour checks > > Nit: `HttpRequest.newBuilder` Typo fixed. See https://github.com/openjdk/jdk/pull/1059/commits/7928083ae954c44b5357064b86af0e1a3f53588b ------------- PR: https://git.openjdk.java.net/jdk/pull/1059
