Commons IO
  1. Commons IO
  2. IO-343

org.apache.commons.io.comparator Javadoc is inconsistent with real code

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.5
    • Component/s: None
    • Labels:
      None

      Description

      Package org.apache.commons.io.comparator has a lot of inconsistent JavaDocs.
      For example this class org.apache.commons.io.comparator.NameFileComparator
      http://svn.apache.org/viewvc/commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/NameFileComparator.java?view=markup
      has JavaDocs
      List<File> list = ...
      NameFileComparator.NAME_COMPARATOR.sort(list); ....

      File[] array = ...
      NameFileComparator.NAME_INSENSITIVE_REVERSE.sort(array);

      but this will not work because all static members of NameFileComparator declared as Comparator<File> for example
      public static final Comparator<File> NAME_REVERSE = new ReverseComparator(NAME_COMPARATOR);
      public static final Comparator<File> NAME_INSENSITIVE_REVERSE = new ReverseComparator(NAME_INSENSITIVE_COMPARATOR);

      and Comparator class doesn't have the sort() method.

      1. patch.txt
        13 kB
        Igor Lash

        Activity

        Hide
        Gary Gregory added a comment -

        Feel free to provide a patch based on trunk code

        Show
        Gary Gregory added a comment - Feel free to provide a patch based on trunk code
        Hide
        Igor Lash added a comment - - edited

        I really like the way of usage of file comparators as described in JavaDoc, so I have changed code. Patch is attached.

        Show
        Igor Lash added a comment - - edited I really like the way of usage of file comparators as described in JavaDoc, so I have changed code. Patch is attached.
        Hide
        Igor Lash added a comment -

        Fixing inconsistency between code and JavaDocs.

        Show
        Igor Lash added a comment - Fixing inconsistency between code and JavaDocs.
        Hide
        Gary Gregory added a comment -

        This patch breaks compatibility so it could only be considered for a major release. I am not sure we want that now.

        Fixing the Javadoc to match the code is OK to do in trunk now but this patch does not do that.

        What needs to be decided as usual is if it is the code or the docs that should be fixed.

        Show
        Gary Gregory added a comment - This patch breaks compatibility so it could only be considered for a major release. I am not sure we want that now. Fixing the Javadoc to match the code is OK to do in trunk now but this patch does not do that. What needs to be decided as usual is if it is the code or the docs that should be fixed.
        Hide
        Igor Lash added a comment - - edited

        This patch will not break backward compatibility. There will be unnecessary cast if someone used comparator in the way

        ((NameFileComparator)NameFileComparator.NAME_COMPARATOR).sort(list)

        Show
        Igor Lash added a comment - - edited This patch will not break backward compatibility. There will be unnecessary cast if someone used comparator in the way ((NameFileComparator)NameFileComparator.NAME_COMPARATOR).sort(list)
        Hide
        Gary Gregory added a comment -

        Hm, OK, so the Clirr errors are false positives because the class AbstractFileComparator implements Comparator<File>.

        Source compatibility is clearly OK (unless you count compiler warnings for the type casting case above).

        Binary compatibility should be OK too.

        Show
        Gary Gregory added a comment - Hm, OK, so the Clirr errors are false positives because the class AbstractFileComparator implements Comparator<File> . Source compatibility is clearly OK (unless you count compiler warnings for the type casting case above). Binary compatibility should be OK too.
        Hide
        Gary Gregory added a comment -

        Committed revision 1378539. Applied patch and fixed compiler warnings in tests.

        Show
        Gary Gregory added a comment - Committed revision 1378539. Applied patch and fixed compiler warnings in tests.
        Hide
        Sebb added a comment -

        The change is not binary compatible.

        I just compiled the following code against IO 2.4:

        public class CompTest {
            public static void main(String[] a) {
                Comparator<File> comparator = DefaultFileComparator.DEFAULT_COMPARATOR;
            }
        }
        

        This runs fine against IO-2.4, but when run with 2.5-SNAPSHOT it fails with:

        Exception in thread "main" java.lang.NoSuchFieldError: DEFAULT_COMPARATOR
                at CompTest.main(CompTest.java:8)
        

        I think this is because the loader looks for the exact field type when trying to find a field. This is similar to the binary incompatibility that results from changing a method return type from void to int or long.

        The code is source-compatible.

        Show
        Sebb added a comment - The change is not binary compatible . I just compiled the following code against IO 2.4: public class CompTest { public static void main( String [] a) { Comparator<File> comparator = DefaultFileComparator.DEFAULT_COMPARATOR; } } This runs fine against IO-2 .4, but when run with 2.5-SNAPSHOT it fails with: Exception in thread "main" java.lang.NoSuchFieldError: DEFAULT_COMPARATOR at CompTest.main(CompTest.java:8) I think this is because the loader looks for the exact field type when trying to find a field. This is similar to the binary incompatibility that results from changing a method return type from void to int or long. The code is source-compatible.
        Hide
        Sebb added a comment -

        Reverted changes:

        URL: http://svn.apache.org/r1469102
        Log:
        IO-343 org.apache.commons.io.comparator Javadoc is inconsistent with real code.
        Revert r1378539 as it broke binary compatibility

        Modified:
        commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/DefaultFileComparator.java
        commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/DirectoryFileComparator.java
        commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/ExtensionFileComparator.java
        commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/LastModifiedFileComparator.java
        commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/NameFileComparator.java
        commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/PathFileComparator.java
        commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/SizeFileComparator.java
        commons/proper/io/trunk/src/test/java/org/apache/commons/io/comparator/ComparatorAbstractTestCase.java
        commons/proper/io/trunk/src/test/java/org/apache/commons/io/comparator/CompositeFileComparatorTest.java
        commons/proper/io/trunk/src/test/java/org/apache/commons/io/comparator/DefaultFileComparatorTest.java
        commons/proper/io/trunk/src/test/java/org/apache/commons/io/comparator/DirectoryFileComparatorTest.java
        commons/proper/io/trunk/src/test/java/org/apache/commons/io/comparator/ExtensionFileComparatorTest.java
        commons/proper/io/trunk/src/test/java/org/apache/commons/io/comparator/LastModifiedFileComparatorTest.java
        commons/proper/io/trunk/src/test/java/org/apache/commons/io/comparator/NameFileComparatorTest.java
        commons/proper/io/trunk/src/test/java/org/apache/commons/io/comparator/PathFileComparatorTest.java
        commons/proper/io/trunk/src/test/java/org/apache/commons/io/comparator/SizeFileComparatorTest.java

        Show
        Sebb added a comment - Reverted changes: URL: http://svn.apache.org/r1469102 Log: IO-343 org.apache.commons.io.comparator Javadoc is inconsistent with real code. Revert r1378539 as it broke binary compatibility Modified: commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/DefaultFileComparator.java commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/DirectoryFileComparator.java commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/ExtensionFileComparator.java commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/LastModifiedFileComparator.java commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/NameFileComparator.java commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/PathFileComparator.java commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/SizeFileComparator.java commons/proper/io/trunk/src/test/java/org/apache/commons/io/comparator/ComparatorAbstractTestCase.java commons/proper/io/trunk/src/test/java/org/apache/commons/io/comparator/CompositeFileComparatorTest.java commons/proper/io/trunk/src/test/java/org/apache/commons/io/comparator/DefaultFileComparatorTest.java commons/proper/io/trunk/src/test/java/org/apache/commons/io/comparator/DirectoryFileComparatorTest.java commons/proper/io/trunk/src/test/java/org/apache/commons/io/comparator/ExtensionFileComparatorTest.java commons/proper/io/trunk/src/test/java/org/apache/commons/io/comparator/LastModifiedFileComparatorTest.java commons/proper/io/trunk/src/test/java/org/apache/commons/io/comparator/NameFileComparatorTest.java commons/proper/io/trunk/src/test/java/org/apache/commons/io/comparator/PathFileComparatorTest.java commons/proper/io/trunk/src/test/java/org/apache/commons/io/comparator/SizeFileComparatorTest.java
        Hide
        Sebb added a comment -

        URL: http://svn.apache.org/r1469107
        Log:
        IO-343 org.apache.commons.io.comparator Javadoc is inconsistent with real code.
        Fix Javadoc to agree with code

        Modified:
        commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/CompositeFileComparator.java
        commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/DefaultFileComparator.java
        commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/DirectoryFileComparator.java
        commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/ExtensionFileComparator.java
        commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/LastModifiedFileComparator.java
        commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/NameFileComparator.java
        commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/PathFileComparator.java
        commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/SizeFileComparator.java

        Show
        Sebb added a comment - URL: http://svn.apache.org/r1469107 Log: IO-343 org.apache.commons.io.comparator Javadoc is inconsistent with real code. Fix Javadoc to agree with code Modified: commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/CompositeFileComparator.java commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/DefaultFileComparator.java commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/DirectoryFileComparator.java commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/ExtensionFileComparator.java commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/LastModifiedFileComparator.java commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/NameFileComparator.java commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/PathFileComparator.java commons/proper/io/trunk/src/main/java/org/apache/commons/io/comparator/SizeFileComparator.java
        Hide
        Sebb added a comment -

        Javadoc now agrees with code without breaking compat.

        Show
        Sebb added a comment - Javadoc now agrees with code without breaking compat.

          People

          • Assignee:
            Unassigned
            Reporter:
            Igor Lash
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development