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 So I do not know is the best solution for this... > 1392 * @return An stream of resource {@link java.net.URL {@code URL}} > > {@code…} is not needed as @link generates the text in <code>..</code> format. > You can simply write: > {@link java.net.URL URL} > > typo s/An/A I have that fixed those now in the actual revision and hopefully using the correct "a" all the times. > I’m unsure if “resource URL” clearly describes it. What about > @return A stream of {@code URL} representing the location of the resources > with the given name. > > 1399 * @since 1.9 > > should be @since 9. Also this has been fixed - old habits ;-) > 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 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 > Mandy > P.S. process wide - RDP1 starts. This enhancement would need a sponsor and > also request for approval [1]. I’m off for the long weekend and will follow > up next Tuesday on this. > > [1] http://openjdk.java.net/projects/jdk9/fc-extension-process - Patrick
