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

Corrected AsyncReaderWrapperForByteBuffer #33

Merged
merged 2 commits into from
Nov 18, 2015

Conversation

adamretter
Copy link
Contributor

Previously AsyncReaderWrapperForByteBuffer was just a copy of AsyncReaderWrapperForByteArray and was not used by any of the tests.

AsyncReaderWrapperForByteBuffer now uses AsyncByteBufferFeeder as I believe it should.

In addition, all the async tests have been updated to test both ByteBuffer and byte[] code paths.
Unfortunately this leads to quite a bit of code duplication in the tests, this could be reduced if we switched to using JUnit annotations and used @RunWith to inject parameters. We could also further reduce code duplication, using try-with-resources from Java 7, and function references from Java 8.

@dizzzz
Copy link

dizzzz commented Nov 17, 2015

Nice work Adam. I thought the project was not alive any more.

@adamretter
Copy link
Contributor Author

@dizzzz Thanks. I am not sure of the status of this project exactly, but I am using it in a tool for obfuscating XML files (which I hope to release as Open Source) and it is working very well for my needs. I chose it because I was particularly interested in using its asynchronous processing.

Perhaps @cowtowncoder can give us an idea about any official status of the project?

@cowtowncoder
Copy link
Member

@adamretter @dizzzz Project is alive, I just haven't had any need for new features myself (haven't really had need to parse XML myself for couple of years now), so mostly relying on bug reports and contributions like this one.

This looks like a nice improvement; let me read it and should be able to merge soon.

@adamretter
Copy link
Contributor Author

@cowtowncoder Thanks very much.

Eventually, I want to expand my tool to also do JSON, perhaps if I am lucky I might find some time to work on FasterXML/jackson-core#57

@dizzzz
Copy link

dizzzz commented Nov 18, 2015

@cowtowncoder great to know it is still alive. Having the project on github really helps already, more or less active maintenance is great! Then it makes sense to actively contribute .....

@cowtowncoder
Copy link
Member

@adamretter In theory building similar JSON codec with a state machine should be smaller undertaking than Aalto. Unfortunately I haven't had enough time or need for non-blocking JSON decoder, so initial plans haven't gone past planning stages. But who knows.

@dizzzz Yes, I want to actively at least help merge contributions in, as well as fix bugs that are found. So similar to Woodstox, project is still maintained if not very active.

@cowtowncoder
Copy link
Member

One more note: I think upgrade to Java 7 would be ok, esp. for use of diamond patterns. But Java 8 is still bit early as I am bit worried about Android compatibility -- apparently Java 7 language constructors are ok (but not JDK classes necessarily).

cowtowncoder added a commit that referenced this pull request Nov 18, 2015
Corrected AsyncReaderWrapperForByteBuffer
@cowtowncoder cowtowncoder merged commit 4c6ca3a into FasterXML:master Nov 18, 2015
@adamretter
Copy link
Contributor Author

@cowtowncoder Okay cool. I am not overly concerned about leaving Java 6 behind, if you want Android compatibility I think you are pretty much stuck there in the medium term. As I understand it Android only supports some features of Java 7 rather than all.

@cowtowncoder
Copy link
Member

@adamretter Yes, wrt JDK. For now, I think I'd like to keep code itself Java 6. Perhaps I should go ahead and release official 1.0, and sometime after that increase base level to Java 7. That way we'd have older branch for users on Android (with possibility to patch critical bugs if any) as a fallback.

@dizzzz
Copy link

dizzzz commented Nov 23, 2015

If the code base is stable, I'd vote for a 1.0 :) it will provide additional community acceptance anyway...

You could keep the 1.x compatible with java6 (sustaining?) and have active development in a 2.x release supporting java7 or even java8 ?

@adamretter
Copy link
Contributor Author

@cowtowncoder @dizzzz Yes I will need a release (1.0 or 0.7) ideally after #34

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

Successfully merging this pull request may close these issues.

3 participants