-
Notifications
You must be signed in to change notification settings - Fork 18
fix: tail_file read from the ending not beginning #70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
`newline_positions` is already in reverse order
hashemix
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jingangdidi , thanks for the contribution.
The change in the logic is incorrect, if you revert that change we can merge the other change to the code doc
src/fs_service/io/read.rs
Outdated
| 0 // Read from start if fewer than n lines | ||
| } else { | ||
| *newline_positions.get(line_count - n).unwrap_or(&0) + 1 | ||
| *newline_positions.get(n).unwrap_or(&0) + 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an invalid change breaking the functionality of the TailFile.
For instance, if a file has 10 lines, and we want to get tail 2 , we need to return lines , starting from line index 8 , which is (line_count - n) = 10 - 2 = 8
If you run the tests on your branch, you will see the tests that will fail:
cargo test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I know what you mean.
The newline_positions is reversed, so index 8 will get the eighth to last line, this will return line 3 and line 4, not line 9 and line10.
5 is a special number, so it can pass the test_tail_file_normal test, I add a new line.
| /// The path of the file to get information for. | ||
| pub path: String, | ||
| /// The number of lines to read from the beginning of the file. | ||
| /// The number of lines to read from the ending of the file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a valid change, thanks 👍
5 is a special number, so it can pass the `test_tail_file_normal` test, I add a new line
hashemix
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, thanks
📌 Summary
tail_file.rs: read from theendingof file notbeginning.read.rs:tail_filereturn the first n lines, not last n lines,newline_positionsis already in reverse order.✨ Changes Made
change
beginningtoendingin TailFile struct.change
get(line_count - n)toget(n)intail_file