You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by zmacomber <gi...@git.apache.org> on 2018/09/28 22:17:57 UTC

[GitHub] commons-lang pull request #358: ClassUtils.getBaseClasses(desiredBase, packa...

GitHub user zmacomber opened a pull request:

    https://github.com/apache/commons-lang/pull/358

    ClassUtils.getBaseClasses(desiredBase, packageName)

    Hello,
    
    I have been using a method in my company's (avadasoftware.com) code base that I created for getting a list of classes that implement an interface. These classes reside in a particular package that I query.  I use this method in my code base in the following manner:
    
    List<FooInterface> fooClasses = ClassUtils.getBaseClasses(FooInterface.class, "com.avada.foo");
    
    I would like to put this in Apache's ClassUtils to move this generic code out of our code base and into what I believe is a more appropriate place.  I'm striving to replace util/generic code in our code base with 3rd party libs as I believe it will be better tested/supported/enhanced/etc... in the OSS world.
    
    I have written several tests that are passing to test exceptional behavior and also normal path behavior.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/zmacomber/commons-lang master

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/commons-lang/pull/358.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #358
    
----
commit 9158247b6baaee11f1c7259d549314d1e2b0bcb3
Author: Zack Macomber <za...@...>
Date:   2018-09-28T22:01:39Z

    Added 'public static <T> List<T> getBaseClasses(final Class<T> desiredBase, final String packageName)' which returns a list of base classes/interfaces that match the supplied type underneath the supplied package

----


---

[GitHub] commons-lang pull request #358: ClassUtils.getBaseClasses(desiredBase, packa...

Posted by aaabramov <gi...@git.apache.org>.
Github user aaabramov commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/358#discussion_r226900283
  
    --- Diff: src/main/java/org/apache/commons/lang3/ClassUtils.java ---
    @@ -1059,6 +1067,62 @@ public static boolean isInnerClass(final Class<?> cls) {
             return getClass(loader, className, initialize);
         }
     
    +    /**
    +     * Returns a list of base classes/interfaces underneath the supplied package
    +     * This method only retrieves base classes/interfaces that have children that can be instantiated
    +     * via a no-args constructor
    --- End diff --
    
    This line could be improved. 
    At least, interfaces has no constructors.


---

[GitHub] commons-lang pull request #358: ClassUtils.getBaseClasses(desiredBase, packa...

Posted by sudeendra-nadager <gi...@git.apache.org>.
Github user sudeendra-nadager commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/358#discussion_r225263101
  
    --- Diff: src/main/java/org/apache/commons/lang3/ClassUtils.java ---
    @@ -1059,6 +1066,69 @@ public static boolean isInnerClass(final Class<?> cls) {
             return getClass(loader, className, initialize);
         }
     
    +    /**
    +     * Returns a list of base classes/interfaces underneath the supplied package
    +     * This method only retrieves base classes/interfaces that have children that can be instantiated
    +     * via a no-args constructor
    +     * The class loader is retrieved via Thread.currentThread().getContextClassLoader()
    +     * This only retrieves base classes/interfaces directly underneath the supplied package
    +     *
    +     * @param desiredBase the desired base class/interface to retrieve
    +     * @param packageName the package name in the standard import format (i.e. "java.lang.String")
    +     * @param <T> The desired base class or interface type to retrieve
    +     * @return a list of base classes/interfaces that match the supplied type underneath the supplied package
    +     * @throws URISyntaxException if the packageName is not found in the class loader
    +     * @throws IOException if an I/O error occurs in getting a new directory stream
    +     * @throws IllegalArgumentException if the desiredBase or packageName are invalid
    +     */
    +    public static <T> List<T> getBaseClasses(final Class<T> desiredBase, final String packageName)
    +            throws URISyntaxException, IOException, IllegalArgumentException {
    +
    +        if (desiredBase == null) {
    +            throw new IllegalArgumentException("desiredBase must not be null");
    +        }
    +
    +        if (StringUtils.isBlank(packageName)) {
    +            throw new IllegalArgumentException("packageName must not be blank");
    +        }
    +
    +        if (packageName.contains("/")) {
    --- End diff --
    
    might be better to validate against a regex. 


---

[GitHub] commons-lang issue #358: ClassUtils.getBaseClasses(desiredBase, packageName)

Posted by zmacomber <gi...@git.apache.org>.
Github user zmacomber commented on the issue:

    https://github.com/apache/commons-lang/pull/358
  
    > Should we overload `getBaseClasses` with the following:
    > 
    > * `ClassUtils.getBaseClasses(final Class<T> desiredBase, final String packageName, ClassLoader classLoader)`
    > * `ClassUtils.getBaseClasses(final Class<T> desiredBase, final Package package)`
    >   ?
    
    I have implemented this



---

[GitHub] commons-lang pull request #358: ClassUtils.getBaseClasses(desiredBase, packa...

Posted by zmacomber <gi...@git.apache.org>.
Github user zmacomber commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/358#discussion_r225508989
  
    --- Diff: src/main/java/org/apache/commons/lang3/ClassUtils.java ---
    @@ -1059,6 +1066,69 @@ public static boolean isInnerClass(final Class<?> cls) {
             return getClass(loader, className, initialize);
         }
     
    +    /**
    +     * Returns a list of base classes/interfaces underneath the supplied package
    +     * This method only retrieves base classes/interfaces that have children that can be instantiated
    +     * via a no-args constructor
    +     * The class loader is retrieved via Thread.currentThread().getContextClassLoader()
    +     * This only retrieves base classes/interfaces directly underneath the supplied package
    +     *
    +     * @param desiredBase the desired base class/interface to retrieve
    +     * @param packageName the package name in the standard import format (i.e. "java.lang.String")
    +     * @param <T> The desired base class or interface type to retrieve
    +     * @return a list of base classes/interfaces that match the supplied type underneath the supplied package
    +     * @throws URISyntaxException if the packageName is not found in the class loader
    +     * @throws IOException if an I/O error occurs in getting a new directory stream
    +     * @throws IllegalArgumentException if the desiredBase or packageName are invalid
    +     */
    +    public static <T> List<T> getBaseClasses(final Class<T> desiredBase, final String packageName)
    +            throws URISyntaxException, IOException, IllegalArgumentException {
    +
    +        if (desiredBase == null) {
    +            throw new IllegalArgumentException("desiredBase must not be null");
    +        }
    +
    +        if (StringUtils.isBlank(packageName)) {
    +            throw new IllegalArgumentException("packageName must not be blank");
    +        }
    +
    +        if (packageName.contains("/")) {
    --- End diff --
    
    Will do


---

[GitHub] commons-lang pull request #358: ClassUtils.getBaseClasses(desiredBase, packa...

Posted by zmacomber <gi...@git.apache.org>.
Github user zmacomber commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/358#discussion_r225537382
  
    --- Diff: src/test/java/org/apache/commons/lang3/ClassUtilsTest.java ---
    @@ -1202,6 +1206,56 @@ public void testGetInnerClass() throws ClassNotFoundException {
             assertEquals( Inner.DeeplyNested.class, ClassUtils.getClass( "org.apache.commons.lang3.ClassUtilsTest$Inner.DeeplyNested" ) );
         }
     
    +    @Test
    +    public void testGetBaseClassesExtends() throws Exception {
    +        List<AnotherParent> classes = ClassUtils.getBaseClasses(AnotherParent.class, "org.apache.commons.lang3.reflect.testbed");
    +        assertTrue(classes.size() > 0);
    +    }
    +
    +    @Test
    +    public void testGetBaseClassesAbstract() throws Exception {
    +        List<AbstractConcurrentInitializerTest> classes = ClassUtils.getBaseClasses(AbstractConcurrentInitializerTest.class, "org.apache.commons.lang3.concurrent");
    +        assertTrue(classes.size() > 0);
    --- End diff --
    
    This is a good idea but the Objects under test are hard to verify...I believe the getBaseClasses() will fail if the Objects returned are invalid.


---

[GitHub] commons-lang pull request #358: ClassUtils.getBaseClasses(desiredBase, packa...

Posted by zmacomber <gi...@git.apache.org>.
Github user zmacomber commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/358#discussion_r225508754
  
    --- Diff: src/test/java/org/apache/commons/lang3/ClassUtilsTest.java ---
    @@ -1202,6 +1206,56 @@ public void testGetInnerClass() throws ClassNotFoundException {
             assertEquals( Inner.DeeplyNested.class, ClassUtils.getClass( "org.apache.commons.lang3.ClassUtilsTest$Inner.DeeplyNested" ) );
         }
     
    +    @Test
    +    public void testGetBaseClassesExtends() throws Exception {
    +        List<AnotherParent> classes = ClassUtils.getBaseClasses(AnotherParent.class, "org.apache.commons.lang3.reflect.testbed");
    +        assertTrue(classes.size() > 0);
    --- End diff --
    
    This is a good idea


---

[GitHub] commons-lang pull request #358: ClassUtils.getBaseClasses(desiredBase, packa...

Posted by zmacomber <gi...@git.apache.org>.
Github user zmacomber commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/358#discussion_r225522003
  
    --- Diff: src/main/java/org/apache/commons/lang3/ClassUtils.java ---
    @@ -1059,6 +1066,69 @@ public static boolean isInnerClass(final Class<?> cls) {
             return getClass(loader, className, initialize);
         }
     
    +    /**
    +     * Returns a list of base classes/interfaces underneath the supplied package
    +     * This method only retrieves base classes/interfaces that have children that can be instantiated
    +     * via a no-args constructor
    +     * The class loader is retrieved via Thread.currentThread().getContextClassLoader()
    +     * This only retrieves base classes/interfaces directly underneath the supplied package
    +     *
    +     * @param desiredBase the desired base class/interface to retrieve
    +     * @param packageName the package name in the standard import format (i.e. "java.lang.String")
    +     * @param <T> The desired base class or interface type to retrieve
    +     * @return a list of base classes/interfaces that match the supplied type underneath the supplied package
    +     * @throws URISyntaxException if the packageName is not found in the class loader
    +     * @throws IOException if an I/O error occurs in getting a new directory stream
    +     * @throws IllegalArgumentException if the desiredBase or packageName are invalid
    +     */
    +    public static <T> List<T> getBaseClasses(final Class<T> desiredBase, final String packageName)
    +            throws URISyntaxException, IOException, IllegalArgumentException {
    +
    +        if (desiredBase == null) {
    +            throw new IllegalArgumentException("desiredBase must not be null");
    +        }
    +
    +        if (StringUtils.isBlank(packageName)) {
    +            throw new IllegalArgumentException("packageName must not be blank");
    +        }
    +
    +        if (packageName.contains("/")) {
    --- End diff --
    
    On 2nd thoughts, think I'll just take out "packageName.contains('/')".  The packageName will be checked below that call.


---

[GitHub] commons-lang pull request #358: ClassUtils.getBaseClasses(desiredBase, packa...

Posted by zmacomber <gi...@git.apache.org>.
Github user zmacomber commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/358#discussion_r228140306
  
    --- Diff: src/main/java/org/apache/commons/lang3/ClassUtils.java ---
    @@ -1059,6 +1067,62 @@ public static boolean isInnerClass(final Class<?> cls) {
             return getClass(loader, className, initialize);
         }
     
    +    /**
    +     * Returns a list of base classes/interfaces underneath the supplied package
    +     * This method only retrieves base classes/interfaces that have children that can be instantiated
    +     * via a no-args constructor
    +     * The class loader is retrieved via Thread.currentThread().getContextClassLoader()
    +     * This only retrieves base classes/interfaces directly underneath the supplied package
    +     *
    +     * @param desiredBase the desired base class/interface to retrieve
    +     * @param packageName the package name in the standard import format (i.e. "java.lang.String")
    +     * @param <T> The desired base class or interface type to retrieve
    +     * @return a list of base classes/interfaces that match the supplied type underneath the supplied package
    +     * @throws IllegalArgumentException if the desiredBase or packageName are invalid
    +     * @throws IOException if an I/O error occurs in getting a new directory stream
    +     * @throws NullPointerException if desiredBase or url are null
    +     * @throws URISyntaxException if the generated url can't be converted to a URI
    +     */
    +    public static <T> List<T> getBaseClasses(final Class<T> desiredBase, final String packageName)
    +            throws IllegalArgumentException, IOException, NullPointerException, URISyntaxException  {
    +
    +        Objects.requireNonNull(desiredBase, "desiredBase must not be null");
    +
    +        if (StringUtils.isBlank(packageName)) {
    +            throw new IllegalArgumentException("packageName must not be blank");
    +        }
    +
    +        ClassLoader classLoader = Thread.currentThread().getContextClassLoader();
    +        URL url = classLoader.getResource(packageName.replaceAll("[.]", "/"));
    +        Objects.requireNonNull(url, "supplied package not found");
    +
    +        Path classesPath = Paths.get(url.toURI());
    +
    +        List<T> classes = new ArrayList<>();
    +
    +        try (DirectoryStream<Path> stream = Files.newDirectoryStream(classesPath)) {
    +            for (Path file: stream) {
    +                Path pathFileName = file.getFileName();
    +                if (( ! Files.isDirectory(file)) && (pathFileName != null)) {
    +                    String fullClassName = packageName + "." +
    +                                           pathFileName.toString().replace(".class", "");
    +
    +                    // Only add classes that can be instantiated via newInstance()
    +                    try {
    +                        Object obj = Class.forName(fullClassName).newInstance();
    --- End diff --
    
    To confirm that the class can actually be instantiated via the default constructor.  Notice the Exception comment and the comment above "try {"


---

[GitHub] commons-lang pull request #358: ClassUtils.getBaseClasses(desiredBase, packa...

Posted by zmacomber <gi...@git.apache.org>.
Github user zmacomber commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/358#discussion_r225537457
  
    --- Diff: src/test/java/org/apache/commons/lang3/ClassUtilsTest.java ---
    @@ -1202,6 +1206,56 @@ public void testGetInnerClass() throws ClassNotFoundException {
             assertEquals( Inner.DeeplyNested.class, ClassUtils.getClass( "org.apache.commons.lang3.ClassUtilsTest$Inner.DeeplyNested" ) );
         }
     
    +    @Test
    +    public void testGetBaseClassesExtends() throws Exception {
    +        List<AnotherParent> classes = ClassUtils.getBaseClasses(AnotherParent.class, "org.apache.commons.lang3.reflect.testbed");
    +        assertTrue(classes.size() > 0);
    --- End diff --
    
    This is a good idea but the Objects under test are hard to verify...I believe the getBaseClasses() will fail if the Objects returned are invalid.


---

[GitHub] commons-lang pull request #358: ClassUtils.getBaseClasses(desiredBase, packa...

Posted by sudeendra-nadager <gi...@git.apache.org>.
Github user sudeendra-nadager commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/358#discussion_r225264152
  
    --- Diff: src/main/java/org/apache/commons/lang3/ClassUtils.java ---
    @@ -1059,6 +1066,69 @@ public static boolean isInnerClass(final Class<?> cls) {
             return getClass(loader, className, initialize);
         }
     
    +    /**
    +     * Returns a list of base classes/interfaces underneath the supplied package
    +     * This method only retrieves base classes/interfaces that have children that can be instantiated
    +     * via a no-args constructor
    +     * The class loader is retrieved via Thread.currentThread().getContextClassLoader()
    +     * This only retrieves base classes/interfaces directly underneath the supplied package
    +     *
    +     * @param desiredBase the desired base class/interface to retrieve
    +     * @param packageName the package name in the standard import format (i.e. "java.lang.String")
    +     * @param <T> The desired base class or interface type to retrieve
    +     * @return a list of base classes/interfaces that match the supplied type underneath the supplied package
    +     * @throws URISyntaxException if the packageName is not found in the class loader
    +     * @throws IOException if an I/O error occurs in getting a new directory stream
    +     * @throws IllegalArgumentException if the desiredBase or packageName are invalid
    +     */
    +    public static <T> List<T> getBaseClasses(final Class<T> desiredBase, final String packageName)
    +            throws URISyntaxException, IOException, IllegalArgumentException {
    +
    +        if (desiredBase == null) {
    +            throw new IllegalArgumentException("desiredBase must not be null");
    +        }
    +
    +        if (StringUtils.isBlank(packageName)) {
    +            throw new IllegalArgumentException("packageName must not be blank");
    +        }
    +
    +        if (packageName.contains("/")) {
    +            throw new IllegalArgumentException("packageName is not properly formatted (i.e. 'java.lang.String')");
    +        }
    +
    +        ClassLoader classLoader = Thread.currentThread().getContextClassLoader();
    +        URL url = classLoader.getResource(packageName.replaceAll("[.]", "/"));
    +        if (url == null) {
    +            throw new URISyntaxException(packageName, "Supplied package not found");
    +        }
    +
    +        Path classesPath = Paths.get(url.toURI());
    +
    +        List<T> classes = new ArrayList<>();
    +
    +        try (DirectoryStream<Path> stream = Files.newDirectoryStream(classesPath)) {
    +            for (Path file: stream) {
    +                Path pathFileName = file.getFileName();
    +                if (( ! Files.isDirectory(file)) && (pathFileName != null)) {
    +                    String fullClassName = packageName + "." +
    +                                           pathFileName.toString().replace(".class", "");
    +
    +                    // Only add classes that can be instantiated via newInstance()
    +                    try {
    +                        Object obj = Class.forName(fullClassName).newInstance();
    +                        if (desiredBase.isInstance(obj)) {
    --- End diff --
    
    IMO - use of instanceof operator is preferred over method call.


---

[GitHub] commons-lang issue #358: ClassUtils.getBaseClasses(desiredBase, packageName)

Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:

    https://github.com/apache/commons-lang/pull/358
  
    
    [![Coverage Status](https://coveralls.io/builds/19280773/badge)](https://coveralls.io/builds/19280773)
    
    Coverage increased (+0.008%) to 95.26% when pulling **4233117365c74a502a576c029eebf5c18bbe1f8b on zmacomber:master** into **69e843890c09861a168c6fe77d63fc72f0c73195 on apache:master**.



---

[GitHub] commons-lang pull request #358: ClassUtils.getBaseClasses(desiredBase, packa...

Posted by aaabramov <gi...@git.apache.org>.
Github user aaabramov commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/358#discussion_r226900275
  
    --- Diff: src/main/java/org/apache/commons/lang3/ClassUtils.java ---
    @@ -1059,6 +1067,62 @@ public static boolean isInnerClass(final Class<?> cls) {
             return getClass(loader, className, initialize);
         }
     
    +    /**
    +     * Returns a list of base classes/interfaces underneath the supplied package
    --- End diff --
    
    I believe it would be better with dots in the end of sentence.


---

[GitHub] commons-lang pull request #358: ClassUtils.getBaseClasses(desiredBase, packa...

Posted by zmacomber <gi...@git.apache.org>.
Github user zmacomber commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/358#discussion_r225508801
  
    --- Diff: src/test/java/org/apache/commons/lang3/ClassUtilsTest.java ---
    @@ -1202,6 +1206,56 @@ public void testGetInnerClass() throws ClassNotFoundException {
             assertEquals( Inner.DeeplyNested.class, ClassUtils.getClass( "org.apache.commons.lang3.ClassUtilsTest$Inner.DeeplyNested" ) );
         }
     
    +    @Test
    +    public void testGetBaseClassesExtends() throws Exception {
    +        List<AnotherParent> classes = ClassUtils.getBaseClasses(AnotherParent.class, "org.apache.commons.lang3.reflect.testbed");
    +        assertTrue(classes.size() > 0);
    +    }
    +
    +    @Test
    +    public void testGetBaseClassesAbstract() throws Exception {
    +        List<AbstractConcurrentInitializerTest> classes = ClassUtils.getBaseClasses(AbstractConcurrentInitializerTest.class, "org.apache.commons.lang3.concurrent");
    +        assertTrue(classes.size() > 0);
    --- End diff --
    
    This is a good idea


---

[GitHub] commons-lang pull request #358: ClassUtils.getBaseClasses(desiredBase, packa...

Posted by zmacomber <gi...@git.apache.org>.
Github user zmacomber commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/358#discussion_r228178841
  
    --- Diff: src/main/java/org/apache/commons/lang3/ClassUtils.java ---
    @@ -1059,6 +1067,62 @@ public static boolean isInnerClass(final Class<?> cls) {
             return getClass(loader, className, initialize);
         }
     
    +    /**
    +     * Returns a list of base classes/interfaces underneath the supplied package
    +     * This method only retrieves base classes/interfaces that have children that can be instantiated
    +     * via a no-args constructor
    --- End diff --
    
    I have committed changes for this


---

[GitHub] commons-lang issue #358: ClassUtils.getBaseClasses(desiredBase, packageName)

Posted by zmacomber <gi...@git.apache.org>.
Github user zmacomber commented on the issue:

    https://github.com/apache/commons-lang/pull/358
  
    Just checked the Travis CI build and it failed because of an import of java.lang.IllegalArgumentException at the top which is not necessary...


---

[GitHub] commons-lang pull request #358: ClassUtils.getBaseClasses(desiredBase, packa...

Posted by zmacomber <gi...@git.apache.org>.
Github user zmacomber commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/358#discussion_r225282462
  
    --- Diff: src/main/java/org/apache/commons/lang3/ClassUtils.java ---
    @@ -1059,6 +1066,69 @@ public static boolean isInnerClass(final Class<?> cls) {
             return getClass(loader, className, initialize);
         }
     
    +    /**
    +     * Returns a list of base classes/interfaces underneath the supplied package
    +     * This method only retrieves base classes/interfaces that have children that can be instantiated
    +     * via a no-args constructor
    +     * The class loader is retrieved via Thread.currentThread().getContextClassLoader()
    +     * This only retrieves base classes/interfaces directly underneath the supplied package
    +     *
    +     * @param desiredBase the desired base class/interface to retrieve
    +     * @param packageName the package name in the standard import format (i.e. "java.lang.String")
    +     * @param <T> The desired base class or interface type to retrieve
    +     * @return a list of base classes/interfaces that match the supplied type underneath the supplied package
    +     * @throws URISyntaxException if the packageName is not found in the class loader
    +     * @throws IOException if an I/O error occurs in getting a new directory stream
    +     * @throws IllegalArgumentException if the desiredBase or packageName are invalid
    +     */
    +    public static <T> List<T> getBaseClasses(final Class<T> desiredBase, final String packageName)
    +            throws URISyntaxException, IOException, IllegalArgumentException {
    +
    +        if (desiredBase == null) {
    +            throw new IllegalArgumentException("desiredBase must not be null");
    +        }
    +
    +        if (StringUtils.isBlank(packageName)) {
    +            throw new IllegalArgumentException("packageName must not be blank");
    +        }
    +
    +        if (packageName.contains("/")) {
    +            throw new IllegalArgumentException("packageName is not properly formatted (i.e. 'java.lang.String')");
    +        }
    +
    +        ClassLoader classLoader = Thread.currentThread().getContextClassLoader();
    +        URL url = classLoader.getResource(packageName.replaceAll("[.]", "/"));
    +        if (url == null) {
    +            throw new URISyntaxException(packageName, "Supplied package not found");
    +        }
    +
    +        Path classesPath = Paths.get(url.toURI());
    +
    +        List<T> classes = new ArrayList<>();
    +
    +        try (DirectoryStream<Path> stream = Files.newDirectoryStream(classesPath)) {
    +            for (Path file: stream) {
    +                Path pathFileName = file.getFileName();
    +                if (( ! Files.isDirectory(file)) && (pathFileName != null)) {
    +                    String fullClassName = packageName + "." +
    +                                           pathFileName.toString().replace(".class", "");
    +
    +                    // Only add classes that can be instantiated via newInstance()
    --- End diff --
    
    The JavaDoc explicitly states that only classes that can be instantiated via a default ctor can be used:
    
    "This method only retrieves base classes/interfaces that have children that can be instantiated via a no-args constructor"
    
    If the class can't be instantiated, an Exception is thrown and it won't be added to the list of returned classes:
    ```
    catch (Exception e) {
        // Class was not instantiable via newInstance()
    }
    ```


---

[GitHub] commons-lang pull request #358: ClassUtils.getBaseClasses(desiredBase, packa...

Posted by zmacomber <gi...@git.apache.org>.
Github user zmacomber commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/358#discussion_r228178951
  
    --- Diff: src/main/java/org/apache/commons/lang3/ClassUtils.java ---
    @@ -1059,6 +1067,62 @@ public static boolean isInnerClass(final Class<?> cls) {
             return getClass(loader, className, initialize);
         }
     
    +    /**
    +     * Returns a list of base classes/interfaces underneath the supplied package
    --- End diff --
    
    I have committed changes for this


---

[GitHub] commons-lang pull request #358: ClassUtils.getBaseClasses(desiredBase, packa...

Posted by zmacomber <gi...@git.apache.org>.
Github user zmacomber commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/358#discussion_r225283052
  
    --- Diff: src/main/java/org/apache/commons/lang3/ClassUtils.java ---
    @@ -1059,6 +1066,69 @@ public static boolean isInnerClass(final Class<?> cls) {
             return getClass(loader, className, initialize);
         }
     
    +    /**
    +     * Returns a list of base classes/interfaces underneath the supplied package
    +     * This method only retrieves base classes/interfaces that have children that can be instantiated
    +     * via a no-args constructor
    +     * The class loader is retrieved via Thread.currentThread().getContextClassLoader()
    +     * This only retrieves base classes/interfaces directly underneath the supplied package
    +     *
    +     * @param desiredBase the desired base class/interface to retrieve
    +     * @param packageName the package name in the standard import format (i.e. "java.lang.String")
    +     * @param <T> The desired base class or interface type to retrieve
    +     * @return a list of base classes/interfaces that match the supplied type underneath the supplied package
    +     * @throws URISyntaxException if the packageName is not found in the class loader
    +     * @throws IOException if an I/O error occurs in getting a new directory stream
    +     * @throws IllegalArgumentException if the desiredBase or packageName are invalid
    +     */
    +    public static <T> List<T> getBaseClasses(final Class<T> desiredBase, final String packageName)
    +            throws URISyntaxException, IOException, IllegalArgumentException {
    +
    +        if (desiredBase == null) {
    +            throw new IllegalArgumentException("desiredBase must not be null");
    +        }
    +
    +        if (StringUtils.isBlank(packageName)) {
    +            throw new IllegalArgumentException("packageName must not be blank");
    +        }
    +
    +        if (packageName.contains("/")) {
    +            throw new IllegalArgumentException("packageName is not properly formatted (i.e. 'java.lang.String')");
    +        }
    +
    +        ClassLoader classLoader = Thread.currentThread().getContextClassLoader();
    +        URL url = classLoader.getResource(packageName.replaceAll("[.]", "/"));
    +        if (url == null) {
    +            throw new URISyntaxException(packageName, "Supplied package not found");
    +        }
    +
    +        Path classesPath = Paths.get(url.toURI());
    +
    +        List<T> classes = new ArrayList<>();
    +
    +        try (DirectoryStream<Path> stream = Files.newDirectoryStream(classesPath)) {
    +            for (Path file: stream) {
    +                Path pathFileName = file.getFileName();
    +                if (( ! Files.isDirectory(file)) && (pathFileName != null)) {
    +                    String fullClassName = packageName + "." +
    +                                           pathFileName.toString().replace(".class", "");
    +
    +                    // Only add classes that can be instantiated via newInstance()
    +                    try {
    +                        Object obj = Class.forName(fullClassName).newInstance();
    +                        if (desiredBase.isInstance(obj)) {
    --- End diff --
    
    instanceof and isInstance() are the same.  You use instanceof when you know the actual class type which, in this case, is unknown.


---

[GitHub] commons-lang pull request #358: ClassUtils.getBaseClasses(desiredBase, packa...

Posted by aaabramov <gi...@git.apache.org>.
Github user aaabramov commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/358#discussion_r226900323
  
    --- Diff: src/main/java/org/apache/commons/lang3/ClassUtils.java ---
    @@ -1059,6 +1067,62 @@ public static boolean isInnerClass(final Class<?> cls) {
             return getClass(loader, className, initialize);
         }
     
    +    /**
    +     * Returns a list of base classes/interfaces underneath the supplied package
    +     * This method only retrieves base classes/interfaces that have children that can be instantiated
    +     * via a no-args constructor
    +     * The class loader is retrieved via Thread.currentThread().getContextClassLoader()
    +     * This only retrieves base classes/interfaces directly underneath the supplied package
    +     *
    +     * @param desiredBase the desired base class/interface to retrieve
    +     * @param packageName the package name in the standard import format (i.e. "java.lang.String")
    +     * @param <T> The desired base class or interface type to retrieve
    +     * @return a list of base classes/interfaces that match the supplied type underneath the supplied package
    +     * @throws IllegalArgumentException if the desiredBase or packageName are invalid
    +     * @throws IOException if an I/O error occurs in getting a new directory stream
    +     * @throws NullPointerException if desiredBase or url are null
    +     * @throws URISyntaxException if the generated url can't be converted to a URI
    +     */
    +    public static <T> List<T> getBaseClasses(final Class<T> desiredBase, final String packageName)
    +            throws IllegalArgumentException, IOException, NullPointerException, URISyntaxException  {
    +
    +        Objects.requireNonNull(desiredBase, "desiredBase must not be null");
    +
    +        if (StringUtils.isBlank(packageName)) {
    +            throw new IllegalArgumentException("packageName must not be blank");
    +        }
    +
    +        ClassLoader classLoader = Thread.currentThread().getContextClassLoader();
    +        URL url = classLoader.getResource(packageName.replaceAll("[.]", "/"));
    +        Objects.requireNonNull(url, "supplied package not found");
    +
    +        Path classesPath = Paths.get(url.toURI());
    +
    +        List<T> classes = new ArrayList<>();
    +
    +        try (DirectoryStream<Path> stream = Files.newDirectoryStream(classesPath)) {
    +            for (Path file: stream) {
    +                Path pathFileName = file.getFileName();
    +                if (( ! Files.isDirectory(file)) && (pathFileName != null)) {
    +                    String fullClassName = packageName + "." +
    +                                           pathFileName.toString().replace(".class", "");
    +
    +                    // Only add classes that can be instantiated via newInstance()
    +                    try {
    +                        Object obj = Class.forName(fullClassName).newInstance();
    --- End diff --
    
    Why do we need to call `newInstance()` on `Class<?>` if we return just `List<Class<?>>`?


---

[GitHub] commons-lang pull request #358: ClassUtils.getBaseClasses(desiredBase, packa...

Posted by grimreaper <gi...@git.apache.org>.
Github user grimreaper commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/358#discussion_r225309090
  
    --- Diff: src/main/java/org/apache/commons/lang3/ClassUtils.java ---
    @@ -1059,6 +1066,69 @@ public static boolean isInnerClass(final Class<?> cls) {
             return getClass(loader, className, initialize);
         }
     
    +    /**
    +     * Returns a list of base classes/interfaces underneath the supplied package
    +     * This method only retrieves base classes/interfaces that have children that can be instantiated
    +     * via a no-args constructor
    +     * The class loader is retrieved via Thread.currentThread().getContextClassLoader()
    +     * This only retrieves base classes/interfaces directly underneath the supplied package
    +     *
    +     * @param desiredBase the desired base class/interface to retrieve
    +     * @param packageName the package name in the standard import format (i.e. "java.lang.String")
    +     * @param <T> The desired base class or interface type to retrieve
    +     * @return a list of base classes/interfaces that match the supplied type underneath the supplied package
    +     * @throws URISyntaxException if the packageName is not found in the class loader
    +     * @throws IOException if an I/O error occurs in getting a new directory stream
    +     * @throws IllegalArgumentException if the desiredBase or packageName are invalid
    +     */
    +    public static <T> List<T> getBaseClasses(final Class<T> desiredBase, final String packageName)
    +            throws URISyntaxException, IOException, IllegalArgumentException {
    +
    +        if (desiredBase == null) {
    --- End diff --
    
    Prefer Objects.requireNonNull instead of explicit `if` checks.


---

[GitHub] commons-lang pull request #358: ClassUtils.getBaseClasses(desiredBase, packa...

Posted by aaabramov <gi...@git.apache.org>.
Github user aaabramov commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/358#discussion_r226899975
  
    --- Diff: src/main/java/org/apache/commons/lang3/ClassUtils.java ---
    @@ -1059,6 +1067,62 @@ public static boolean isInnerClass(final Class<?> cls) {
             return getClass(loader, className, initialize);
         }
     
    +    /**
    +     * Returns a list of base classes/interfaces underneath the supplied package
    --- End diff --
    
    I believe it would be better with dots in the end of sentence.


---

[GitHub] commons-lang issue #358: ClassUtils.getBaseClasses(desiredBase, packageName)

Posted by aaabramov <gi...@git.apache.org>.
Github user aaabramov commented on the issue:

    https://github.com/apache/commons-lang/pull/358
  
    Should we overload `getBaseClasses` with the following:
    - `ClassUtils.getBaseClasses(final Class<T> desiredBase, final String packageName, ClassLoader classLoader)`
    - `ClassUtils.getBaseClasses(final Class<T> desiredBase, final Package package)`
    ?


---

[GitHub] commons-lang pull request #358: ClassUtils.getBaseClasses(desiredBase, packa...

Posted by sudeendra-nadager <gi...@git.apache.org>.
Github user sudeendra-nadager commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/358#discussion_r225264946
  
    --- Diff: src/test/java/org/apache/commons/lang3/ClassUtilsTest.java ---
    @@ -1202,6 +1206,56 @@ public void testGetInnerClass() throws ClassNotFoundException {
             assertEquals( Inner.DeeplyNested.class, ClassUtils.getClass( "org.apache.commons.lang3.ClassUtilsTest$Inner.DeeplyNested" ) );
         }
     
    +    @Test
    +    public void testGetBaseClassesExtends() throws Exception {
    +        List<AnotherParent> classes = ClassUtils.getBaseClasses(AnotherParent.class, "org.apache.commons.lang3.reflect.testbed");
    +        assertTrue(classes.size() > 0);
    --- End diff --
    
    should validate each element of the container for better accuracy.


---

[GitHub] commons-lang pull request #358: ClassUtils.getBaseClasses(desiredBase, packa...

Posted by sudeendra-nadager <gi...@git.apache.org>.
Github user sudeendra-nadager commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/358#discussion_r225263720
  
    --- Diff: src/main/java/org/apache/commons/lang3/ClassUtils.java ---
    @@ -1059,6 +1066,69 @@ public static boolean isInnerClass(final Class<?> cls) {
             return getClass(loader, className, initialize);
         }
     
    +    /**
    +     * Returns a list of base classes/interfaces underneath the supplied package
    +     * This method only retrieves base classes/interfaces that have children that can be instantiated
    +     * via a no-args constructor
    +     * The class loader is retrieved via Thread.currentThread().getContextClassLoader()
    +     * This only retrieves base classes/interfaces directly underneath the supplied package
    +     *
    +     * @param desiredBase the desired base class/interface to retrieve
    +     * @param packageName the package name in the standard import format (i.e. "java.lang.String")
    +     * @param <T> The desired base class or interface type to retrieve
    +     * @return a list of base classes/interfaces that match the supplied type underneath the supplied package
    +     * @throws URISyntaxException if the packageName is not found in the class loader
    +     * @throws IOException if an I/O error occurs in getting a new directory stream
    +     * @throws IllegalArgumentException if the desiredBase or packageName are invalid
    +     */
    +    public static <T> List<T> getBaseClasses(final Class<T> desiredBase, final String packageName)
    +            throws URISyntaxException, IOException, IllegalArgumentException {
    +
    +        if (desiredBase == null) {
    +            throw new IllegalArgumentException("desiredBase must not be null");
    +        }
    +
    +        if (StringUtils.isBlank(packageName)) {
    +            throw new IllegalArgumentException("packageName must not be blank");
    +        }
    +
    +        if (packageName.contains("/")) {
    +            throw new IllegalArgumentException("packageName is not properly formatted (i.e. 'java.lang.String')");
    +        }
    +
    +        ClassLoader classLoader = Thread.currentThread().getContextClassLoader();
    +        URL url = classLoader.getResource(packageName.replaceAll("[.]", "/"));
    +        if (url == null) {
    +            throw new URISyntaxException(packageName, "Supplied package not found");
    +        }
    +
    +        Path classesPath = Paths.get(url.toURI());
    +
    +        List<T> classes = new ArrayList<>();
    +
    +        try (DirectoryStream<Path> stream = Files.newDirectoryStream(classesPath)) {
    +            for (Path file: stream) {
    +                Path pathFileName = file.getFileName();
    +                if (( ! Files.isDirectory(file)) && (pathFileName != null)) {
    +                    String fullClassName = packageName + "." +
    +                                           pathFileName.toString().replace(".class", "");
    +
    +                    // Only add classes that can be instantiated via newInstance()
    --- End diff --
    
    what if the derived class does not provide default ctor?


---

[GitHub] commons-lang pull request #358: ClassUtils.getBaseClasses(desiredBase, packa...

Posted by zmacomber <gi...@git.apache.org>.
Github user zmacomber commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/358#discussion_r228178901
  
    --- Diff: src/main/java/org/apache/commons/lang3/ClassUtils.java ---
    @@ -1059,6 +1067,62 @@ public static boolean isInnerClass(final Class<?> cls) {
             return getClass(loader, className, initialize);
         }
     
    +    /**
    +     * Returns a list of base classes/interfaces underneath the supplied package
    --- End diff --
    
    I have committed changes for this


---

[GitHub] commons-lang pull request #358: ClassUtils.getBaseClasses(desiredBase, packa...

Posted by sudeendra-nadager <gi...@git.apache.org>.
Github user sudeendra-nadager commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/358#discussion_r225264987
  
    --- Diff: src/test/java/org/apache/commons/lang3/ClassUtilsTest.java ---
    @@ -1202,6 +1206,56 @@ public void testGetInnerClass() throws ClassNotFoundException {
             assertEquals( Inner.DeeplyNested.class, ClassUtils.getClass( "org.apache.commons.lang3.ClassUtilsTest$Inner.DeeplyNested" ) );
         }
     
    +    @Test
    +    public void testGetBaseClassesExtends() throws Exception {
    +        List<AnotherParent> classes = ClassUtils.getBaseClasses(AnotherParent.class, "org.apache.commons.lang3.reflect.testbed");
    +        assertTrue(classes.size() > 0);
    +    }
    +
    +    @Test
    +    public void testGetBaseClassesAbstract() throws Exception {
    +        List<AbstractConcurrentInitializerTest> classes = ClassUtils.getBaseClasses(AbstractConcurrentInitializerTest.class, "org.apache.commons.lang3.concurrent");
    +        assertTrue(classes.size() > 0);
    --- End diff --
    
    should validate each element of the container for better accuracy.


---

[GitHub] commons-lang pull request #358: ClassUtils.getBaseClasses(desiredBase, packa...

Posted by zmacomber <gi...@git.apache.org>.
Github user zmacomber commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/358#discussion_r225509052
  
    --- Diff: src/main/java/org/apache/commons/lang3/ClassUtils.java ---
    @@ -1059,6 +1066,69 @@ public static boolean isInnerClass(final Class<?> cls) {
             return getClass(loader, className, initialize);
         }
     
    +    /**
    +     * Returns a list of base classes/interfaces underneath the supplied package
    +     * This method only retrieves base classes/interfaces that have children that can be instantiated
    +     * via a no-args constructor
    +     * The class loader is retrieved via Thread.currentThread().getContextClassLoader()
    +     * This only retrieves base classes/interfaces directly underneath the supplied package
    +     *
    +     * @param desiredBase the desired base class/interface to retrieve
    +     * @param packageName the package name in the standard import format (i.e. "java.lang.String")
    +     * @param <T> The desired base class or interface type to retrieve
    +     * @return a list of base classes/interfaces that match the supplied type underneath the supplied package
    +     * @throws URISyntaxException if the packageName is not found in the class loader
    +     * @throws IOException if an I/O error occurs in getting a new directory stream
    +     * @throws IllegalArgumentException if the desiredBase or packageName are invalid
    +     */
    +    public static <T> List<T> getBaseClasses(final Class<T> desiredBase, final String packageName)
    +            throws URISyntaxException, IOException, IllegalArgumentException {
    +
    +        if (desiredBase == null) {
    --- End diff --
    
    Will do


---