IO-543-ReversedLinesFileReader-with-RandowFileAccess-API by JeanPierrePortier · Pull Request #39 · apache/commons-io

JeanPierrePortier · 2017-07-03T13:53:56Z

pull request for ticket: https://issues.apache.org/jira/browse/IO-543

Added getFilePointer and seek API to ReversedLinesFileReader
Added new JUnit tests (ReversedLinesFileReaderTestRandomAccess)

coveralls · 2017-07-03T14:13:33Z

Coverage increased (+0.09%) to 90.086% when pulling a5bc2f8 on JeanPierrePortier:IO-543-ReversedLinesFileReader-with-RandowFileAccess-API into acceb62 on apache:master.

garydgregory

@JeanPierrePortier
Thank you for your PR. May you please rebase on git master and address my comments?

garydgregory · 2021-09-23T18:09:02Z

src/main/java/org/apache/commons/io/input/ReversedLinesFileReader.java

+        seek (totalByteLength);
+
+    }
+    /**


All new public and protected method should have complete Javadoc. Here the first sentence is missing as is the since tag.

garydgregory · 2021-09-23T18:09:25Z

src/main/java/org/apache/commons/io/input/ReversedLinesFileReader.java

+        /**
+         * @return Returns the current offset in this file
+         */
+        public long getFilePointer(){


Formatting is missing a space.

overwrite target file #319. - Based on PR #39 but different. - New assert method leaks input stream; this bug was copied from existing code into a new method. - Use NIO. - Tests don't need to schedule files for deletion on exit when JUnit already manages the directory.

garydgregory · 2022-08-08T15:00:27Z

src/main/java/org/apache/commons/io/input/ReversedLinesFileReader.java

        private byte[] leftOver;

        private int currentLastBytePos;
+        private int mappedFilePointer; // store file pointer position according to currentLastBytePos but take


Just a Javadoc comment instead.

garydgregory · 2022-08-08T15:00:37Z

src/main/java/org/apache/commons/io/input/ReversedLinesFileReader.java

                    line = new String(lineData, encoding);

                    currentLastBytePos = i - newLineMatchByteCount;
+                    mappedFilePointer=i;


Fix formatting.

garydgregory · 2022-08-08T15:00:45Z

src/main/java/org/apache/commons/io/input/ReversedLinesFileReader.java

                leftOver = null;
            }
            currentLastBytePos = -1;
+            mappedFilePointer=0;


Fix formatting

garydgregory

Hi @JeanPierrePortier
Ping?

Jean-Pierre-Portier · 2022-08-10T05:42:53Z

Hi @garydgregory
This is a very old PR and I did not work onto it for a long time now.
Let me sync with sources; please.

Added new JUnit tests (ReversedLinesFileReaderTestRandomAccess)

Jean-Pierre-Portier · 2022-08-11T13:15:39Z

Hello @garydgregory ,
sorry for the delay; tried to sync my mind with current master state (more than 1500 commits behind....)
So just pushed a rebased branch with improved unit tests

garydgregory · 2022-08-11T13:53:36Z

Hello @JeanPierrePortier
Thank you for the update.
You'll want to run a local command line build with just mvn which runs the default Maven goal. This will make sure you do not cause your branch build here to fail by fixing issues before you push your branch. ATM, you have failures.

codecov-commenter · 2022-08-11T13:56:11Z

Codecov Report

Merging #39 (cdf495f) into master (279a4c4) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master      #39      +/-   ##
============================================
- Coverage     85.66%   85.65%   -0.01%     
+ Complexity     3102     3101       -1     
============================================
  Files           204      204              
  Lines          7368     7370       +2     
  Branches        905      904       -1     
============================================
+ Hits           6312     6313       +1     
  Misses          810      810              
- Partials        246      247       +1

Impacted Files	Coverage Δ
src/main/java/org/apache/commons/io/FileUtils.java	`94.42% <100.00%> (-0.04%)`	⬇️
...ache/commons/io/input/ReversedLinesFileReader.java	`90.71% <100.00%> (+3.40%)`	⬆️
.../apache/commons/io/input/ReadAheadInputStream.java	`68.53% <0.00%> (-3.38%)`	⬇️
.../main/java/org/apache/commons/io/input/Tailer.java	`86.06% <0.00%> (+0.49%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Jean-Pierre-Portier · 2022-08-11T14:10:27Z

Yes just seen this (I just launched mvn verify command) before pushing and it is not enough...

garydgregory · 2022-08-11T14:34:38Z

Yes just seen this (I just launched mvn verify command) before pushing and it is not enough...

Just run mvn

Jean-Pierre-Portier · 2022-08-11T15:11:18Z

Last CI run failed for windows env for files not related to this PR.
No idea why and failures should not be related to this PR (I do not have windows host for validation)

garydgregory

Hi @JeanPierrePortier
Please rebase on master where the OS and Java version-specific build failure are fixed; please also address my comments in the PR comments. TY!

garydgregory · 2022-08-13T17:45:00Z

src/main/java/org/apache/commons/io/input/ReversedLinesFileReader.java

+     * Returns the current offset in this file
+     * @return the current offset
+     * @throws IOException if an I/O error occurs.
+     */


New public and protected APIs need an @since tag.

garydgregory · 2022-08-13T17:45:48Z

src/main/java/org/apache/commons/io/input/ReversedLinesFileReader.java

+     * @return the current offset
+     * @throws IOException if an I/O error occurs.
+     */
+    public long getFilePointer() throws IOException {


Why not call the new method position since (1) there are no pointers in Java and (2) it reflects the underlying operation?

garydgregory · 2022-08-13T17:45:57Z

src/main/java/org/apache/commons/io/input/ReversedLinesFileReader.java

+    /**
+     * Sets the file-pointer offset, measured from the beginning of this file, at which the next read occurs.
+     * @param pos the offset value to be set
+     * @throws IOException if an I/O error occurs.


New public and protected APIs need an @SInCE tag.

garydgregory · 2022-08-13T17:46:34Z

src/main/java/org/apache/commons/io/input/ReversedLinesFileReader.java

+     * @param pos the offset value to be set
+     * @throws IOException if an I/O error occurs.
+     */
+    public void seek (long pos) throws IOException {


No space after the method name. Use final where you can.

garydgregory · 2022-08-13T17:58:54Z

src/main/java/org/apache/commons/io/input/ReversedLinesFileReader.java

+        this.currentFilePart = new FilePart(block, blockLength, null);
+
+    }
+}


Files should end in a blank line.

garydgregory · 2022-08-13T17:59:47Z

src/test/java/org/apache/commons/io/input/ReversedLinesFileReaderTestRandomAccess.java

+  @AfterEach
+  public void closeReader() {
+      try {
+          reversedLinesFileReader.close();


Use IOUtils.closeQuietly() instead.

Thank you for your feedbacks @garydgregory
I won't be able to take them before two weeks, but will do it ASAP.

garydgregory

Please see my comments.

garydgregory requested changes Sep 23, 2021

View reviewed changes

garydgregory reviewed Aug 8, 2022

View reviewed changes

garydgregory requested changes Aug 8, 2022

View reviewed changes

Added getFilePointer and seek API to ReversedLinesFileReader

cdf495f

Added new JUnit tests (ReversedLinesFileReaderTestRandomAccess)

IO-543: CI fixes

61f0488

garydgregory requested changes Aug 13, 2022

View reviewed changes

garydgregory requested changes Aug 14, 2022

View reviewed changes

Jean-Pierre-Portier closed this by deleting the head repository Sep 6, 2022

Conversation

JeanPierrePortier commented Jul 3, 2017

Uh oh!

coveralls commented Jul 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Jean-Pierre-Portier commented Aug 10, 2022

Uh oh!

Jean-Pierre-Portier commented Aug 11, 2022

Uh oh!

garydgregory commented Aug 11, 2022

Uh oh!

codecov-commenter commented Aug 11, 2022

Codecov Report

Uh oh!

Jean-Pierre-Portier commented Aug 11, 2022

Uh oh!

garydgregory commented Aug 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jean-Pierre-Portier commented Aug 11, 2022

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Jul 3, 2017 •

edited

Loading

garydgregory commented Aug 11, 2022 •

edited

Loading