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

Allow InputStream or Reader as Parser input #2

Closed
robinst opened this issue Jul 23, 2015 · 9 comments
Closed

Allow InputStream or Reader as Parser input #2

robinst opened this issue Jul 23, 2015 · 9 comments

Comments

@robinst
Copy link
Collaborator

robinst commented Jul 23, 2015

Currently, the only input type is String. In case the input is read from a stream, this means it has to be read into a String first, and then the parser has another copy of the data as block content.

Accepting an InputStream (or Reader?) instead would allow to get rid of one of the copies. DocumentParser already processes the input line by line, so this should be trivial.

@armstnp
Copy link

armstnp commented Jul 26, 2015

What would be the viability of passing in more focused type, e.g. Iterable<String> or a custom interface such as a LineSupplier?

(And yes, the viability of Iterable for this type of abstraction is debatable, too.)

@robinst
Copy link
Collaborator Author

robinst commented Jul 27, 2015

That would make the common case of parsing a file or other stream of characters harder, because the caller would have to adapt a BufferedReader to an Iterable<String>. Also, the contract of an Iterable is that it can be iterated multiple times, which is not the case for a stream.

After some more thinking, I think we should add an overloaded parse with a Reader argument. If it's already a BufferedReader (e.g. from Files.newBufferedReader), we use it as-is, otherwise we wrap it in one. Then read line by line. The caller is responsible for closing it.

I think a Reader is better suited than InputStream, as we don't want to know about the Charset, the caller is responsible for that (also matches spec).

@robinst
Copy link
Collaborator Author

robinst commented Jul 27, 2015

There may also be situations where Iterable<String> makes sense, but we can add that later if needed.

@robinst
Copy link
Collaborator Author

robinst commented Jul 28, 2015

Thanks for working on this, @sentyaev. It would have been nice if you commented here first though, maybe @armstnp would have liked to implement this.

@armstnp
Copy link

armstnp commented Jul 28, 2015

All's good. 👍

robinst added a commit that referenced this issue Aug 8, 2015
#2 Reader to input for Parser
@robinst robinst changed the title Allow InputStream as Parser input Allow InputStream or Reader as Parser input Aug 8, 2015
@robinst
Copy link
Collaborator Author

robinst commented Aug 8, 2015

Implemented with a new method parseReader on Parser, thanks @sentyaev

@robinst robinst closed this as completed Aug 8, 2015
@robinst
Copy link
Collaborator Author

robinst commented Aug 21, 2015

Released in 0.2.0.

@sentyaev
Copy link
Contributor

Great! My first open source project commit.

@robinst
Copy link
Collaborator Author

robinst commented Aug 23, 2015

Oh, congratulations! Hopefully there will be many more :).

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

No branches or pull requests

3 participants