Skip to content
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

added lines() in LineReader #6308

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ayushsinghal90
Copy link
Contributor

  • Added lines() in LinedReader.
  • Modified LineBufferTest class to test lines() function.

Solves #6067

}
@Override
public String next() {
if (!nextLine.equals("") || hasNext()) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The !nextLine.equals("") condition should be already covered by the hasNext() right?

*/
public Stream<String> lines()
{
Iterator<String> iterator = new Iterator<String>() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps using the AbstractIterator would make the logic quite a bit simpler

@@ -14,19 +14,29 @@

package com.google.common.io;

import static com.google.common.base.Preconditions.checkNotNull;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you chance the import order accidentally? Google Java Styleguide

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, fixed.

Copy link

@SplotyCode SplotyCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a project maintainer. I just wrote the things that looked obvious to me. But since you added me as a reviewer i will try my best. But please take the comments as a 'open for discussion' rather then a todo

@@ -121,6 +127,13 @@ private static List<String> readUsingReader(String input, int chunk, boolean asR
return lines;
}

private static Stream<String> readUsingReaderGetLines(String input, int chunk, boolean asReader) {
Readable readable =
asReader ? getChunkedReader(input, chunk) : getChunkedReadable(input, chunk);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When looking through the code base, this is the way they split the ternary operator

Suggested change
asReader ? getChunkedReader(input, chunk) : getChunkedReadable(input, chunk);
Readable readable = asReader
? getChunkedReader(input, chunk)
: getChunkedReadable(input, chunk);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I referred this function readUsingReader.

* this {@code LineReader}.
*
* @return a {@code Stream<String>} providing the lines of text
* described by this {@code LineReader}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the JavaDoc should:

  • state that the caller is in charge of closing the Stream (probably also annotate the method with @MustBeClosed)
  • mention the fact that we throw UncheckedIOException
  • explain how we define a line (maybe just mention that a line has the same semantics as in LineReader#lines)

@ayushsinghal90 ayushsinghal90 force-pushed the issue_6067 branch 2 times, most recently from 05eadd3 to 97bd7a5 Compare January 29, 2023 20:38
*
* @return a {@code Stream<String>} providing the lines of text
* described by this {@code LineReader}
* @throws UncheckedIOException if an I/O error occurs.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method itself does not throw UncheckedIOException right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants