> On Sep 3, 2016, at 12:55 PM, Patrick Reinhart <patr...@reini.net> wrote: > > Hi Mandy, > > The current changes can be found here: > > http://cr.openjdk.java.net/~reinhapa/reviews/8161230/webrev.02 > > > On 02.09.2016 23:11, Mandy Chung wrote: >>> On Sep 2, 2016, at 2:50 AM, Patrick Reinhart <patr...@reini.net> >>> wrote: >>> >>> Updated the existing >>> http://cr.openjdk.java.net/~reinhapa/reviews/8161230/webrev.01 >> ClassLoader::resources returning Stream<URL> is a good addition. >> >> 1386 * {@code IOException} occur getting the next resource element, it >> must be >> 1387 * wrapped into an {@code UncheckedIOException}. >> >> This has been mentioned in the paragraph above. This can be dropped from >> @apiNote. And add: >> >> @throws UncheckedIOException if I/O errors occur >> > There was already an discussion about this with Alan Bateman: > > http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-August/042792.html >
I sample a few existing APIs and they document UncheckedIOException in the method description rather than @throws as Alan described. > So I do not know is the best solution for this… Your method description already covers it. It’s fine. line 1386-1387 in @apiNote is redundant that I still think can be taken out. >> >> I have reservation in adding ClassLoader::systemResources static method. >> >> ClassLoader::getSystemResources is a convenient method that is equivalent to >> calling ClassLoader::getSystemClassLoader().getResources(), since the system >> class loader is never null. This method includes the resources in the >> application’s classpath. >> >> With the new ClassLoader::getPlatformClassLoader() method, a better way to >> get the “system” resources from JDK (not the ones from the classpath), it >> can call: >> ClassLoader::getPlatformClassLoader().resources(); >> >> To get a Stream of URL of the resources visible to system class loader, it >> can call the new resources method: >> ClassLoader::getSystemClassLoader().resources(); >> >> So it doesn’t seem that the proposed systemResources static method is >> necessary. >> > > Hmm... We may have overlooked this, when we started by just looking into the > API changes in general. I did those with Paul Sandoz in more deep in the > following thread: > > http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-August/042752.html > > and this proposal is based on the "go" from Paul here: > > http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-August/043012.html > I file a bug to update the spec ClassLoader::getSystemClassLoader to return non-null: https://bugs.openjdk.java.net/browse/JDK-8165563 >> I suggest to combine the two tests in a single test, ResourcesStreamTest and >> add @bug JBS-id >> >> ResourcesSuccessCase >> >> 46 List<URL> resources = stream.collect(Collectors.toList()); >> 52 URL url = resources.get(0); >> >> You can check the size first. This can be simplified to use >> filter/findFirst to file the expected URL (yes need to call cl.resources >> again that is not an issue). >> > > I have done this also in the new revision testFailure method can be simplified to long count = cl.resources(“the name”).count(); if (count != 1) throw new Exception("expected resource is null or empty"); cl.resources("the name”) .filter(url -> "file:/somefile".equals(url.toExternalForm()) .findFirst() .orElseThrow(…) Mandy
