jonfreedman commented on pull request #32:
URL: https://github.com/apache/commons-io/pull/32#issuecomment-913855307


   I have a feeling I just lifted all the java doc from the underlying methods, 
they may have changed in the last 5 years and I'll also incorporate your 
suggestions. Regarding the constructors/factory methods, personally I would 
prefer to replace the whole lot with a builder but that's obviously quite a 
departure from the existing code, I had designed this PR to be as close as 
possible to the existing but I'm happy to make it 'better' if it would be 
accepted?  ---- On Mon, 06 Sep 2021 19:40:47 +0100  ***@***.***  wrote ----
   @garydgregory requested changes on this pull request.
   
   @jonfreedman
   Thank you for the update.
   Hi, I did a review and scattered my comments throughout.
   
   In src/main/java/org/apache/commons/io/input/Tailer.java:
   > +         * @return  <code>true</code> if and only if the tailable exists;
   +         *          <code>false</code> otherwise
   +         */
   +        boolean exists();
   +
   +        /**
   +         * Tests if this tailable is newer than the specified time 
reference.
   +         *
   +         * @param timeMillis the time reference measured in milliseconds 
since the
   +         *                   epoch (00:00:00 GMT, January 1, 1970).
   +         * @return true if this tailable has been modified after the given 
time reference.
   +         */
   +        boolean isFileNewer(final long timeMillis);
   +
   +        /**
   +         * @param mode the access mode ***@***.*** RandomAccessFile}
   
   The 1st sentence is missing for this method.
   
   
   
   In src/main/java/org/apache/commons/io/input/Tailer.java:
   > @@ -556,4 +784,181 @@ private long readLines(final RandomAccessFile 
reader) throws IOException {
                return rePos;
            }
        }
   +
   +    /**
   +     * Abstraction on ***@***.*** java.io.File} which allows substitution 
of remote files for example using jCIFS
   +     *
   +     * @since 2.12.0
   +     */
   +    public interface Tailable {
   +        /**
   +         * @return  The name of the file denoted by this tailable
   
   The 1st sentence is missing for this method.
   
   
   
   In src/main/java/org/apache/commons/io/input/Tailer.java:
   > +        /**
   +         * @param mode the access mode ***@***.*** RandomAccessFile}
   +         * @return a random access file stream to read from
   +         * @throws FileNotFoundException if the tailable object does not 
exist
   +         */
   +        RandomAccessTailable getRandomAccess(final String mode) throws 
FileNotFoundException;
   +    }
   +
   +    /**
   +     * Abstraction on ***@***.*** java.io.RandomAccessFile} which allows 
substitution of remote files for example using jCIFS
   +     *
   +     * @since 2.12.0
   +     */
   +    public interface RandomAccessTailable extends Closeable {
   +        /**
   +         * Returns the current offset in this tailable.
   
   "Returns" -> "Gets"
   
   
   
   In src/main/java/org/apache/commons/io/input/Tailer.java:
   > +         * @exception  IOException  if an I/O error occurs.
   +         */
   +        long getFilePointer() throws IOException;
   +
   +        /**
   +         * Sets the file-pointer offset, measured from the beginning of this
   +         * tailable, at which the next read or write occurs.  The offset 
may be
   +         * set beyond the end of the tailable. Setting the offset beyond 
the end
   +         * of the tailable does not change the tailable length.  The 
tailable
   +         * length will change only by writing after the offset has been set 
beyond
   +         * the end of the tailable.
   +         *
   +         * @param      pos   the offset position, measured in bytes from the
   +         *                   beginning of the tailable, at which to set the
   +         *                   tailable pointer.
   +         * @exception  IOException  if ***@***.*** pos} is less than
   
   ***@***.***" -> ***@***.***"
   
   
   
   In src/main/java/org/apache/commons/io/input/Tailer.java:
   > +     */
   +    public interface Tailable {
   +        /**
   +         * @return  The name of the file denoted by this tailable
   +         */
   +        String getName();
   +
   +        /**
   +         * @return  The full path of this tailable
   +         */
   +        String getPath();
   +
   +        /**
   +         * Returns the length of this tailable
   +         *
   +         * @return  The length, in bytes, of this tailable, or 
<code>0L</code>
   
   foo -> ***@***.*** foo}
   
   
   
   In src/main/java/org/apache/commons/io/input/Tailer.java:
   > +         * @param timeMillis the time reference measured in milliseconds 
since the
   +         *                   epoch (00:00:00 GMT, January 1, 1970).
   +         * @return true if this tailable has been modified after the given 
time reference.
   +         */
   +        boolean isFileNewer(final long timeMillis);
   +
   +        /**
   +         * @param mode the access mode ***@***.*** RandomAccessFile}
   +         * @return a random access file stream to read from
   +         * @throws FileNotFoundException if the tailable object does not 
exist
   +         */
   +        RandomAccessTailable getRandomAccess(final String mode) throws 
FileNotFoundException;
   +    }
   +
   +    /**
   +     * Abstraction on ***@***.*** java.io.RandomAccessFile} which allows 
substitution of remote files for example using jCIFS
   
   Sentences should end in a period.
   
   
   
   In src/main/java/org/apache/commons/io/input/Tailer.java:
   > +
   +    /**
   +     * Abstraction on ***@***.*** java.io.File} which allows substitution 
of remote files for example using jCIFS
   +     *
   +     * @since 2.12.0
   +     */
   +    public interface Tailable {
   +        /**
   +         * @return  The name of the file denoted by this tailable
   +         */
   +        String getName();
   +
   +        /**
   +         * @return  The full path of this tailable
   +         */
   +        String getPath();
   
   Confusing API name because it does not return a Path; either rename the API 
or make it return a Path.
   
   
   
   In src/main/java/org/apache/commons/io/input/Tailer.java:
   > @@ -556,4 +784,181 @@ private long readLines(final RandomAccessFile 
reader) throws IOException {
                return rePos;
            }
        }
   +
   +    /**
   +     * Abstraction on ***@***.*** java.io.File} which allows substitution 
of remote files for example using jCIFS
   +     *
   +     * @since 2.12.0
   +     */
   +    public interface Tailable {
   +        /**
   +         * @return  The name of the file denoted by this tailable
   +         */
   +        String getName();
   
   The name of... what? the base file name? The full file name?
   
   
   
   In src/main/java/org/apache/commons/io/input/Tailer.java:
   > @@ -191,6 +203,18 @@ public Tailer(final File file, final TailerListener 
listener, final long delayMi
            this(file, listener, delayMillis, false);
        }
    
   +    /**
   
   There are way too many new factory APIs here IMO. For the first cut, let's 
add one and see what other use-cases require in the future. If a delay is 
provided, please make that a Duration, not a long. I'll redo the internals 
after this PR comes in to use a Duration instead of a long. If you add a 
constructor, make it private or package private.
   
   —You are receiving this because you were mentioned.Reply to this email 
directly, view it on GitHub, or unsubscribe.Triage notifications on the go with 
GitHub Mobile for iOS or Android.
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@commons.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


Reply via email to