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

Alternate approach for issue #17 #353

Merged
merged 4 commits into from
Feb 23, 2017
Merged

Conversation

uhhhh2
Copy link
Contributor

@uhhhh2 uhhhh2 commented Feb 9, 2017

I've created a possible approach for addressing Issue #17 .

First, I added the abstract method declaration in JsonGenerator. Next, I added implementations of this method in two of the JsonGenerator implementations. Iin one implementation of the JsonGenerator (WriterBasedJsonGenerator), I created a separate buffer field in the class (using new char[] to avoid calling the same IOContext.allocX method twice which apparently results in an exception). In another implementation of the JsonGenerator (UTF8JsonGenerator), I tried to reuse the (apparently pre-existing) _charBuffer. I am not sure if these are the best approaches for getting a char[] for use as a temporary buffer for reading from a Reader. Then, I added an implementation in the JsonGeneratorDelegate that calls the appropriate function of the delegate instance.

As for a unit test, I copied one of the pre-existing unit test files (StringGenerationTest) and created a new version (StringGenerationReaderTest to avoid deleting the original tests) In StringGenerationReaderTest, the boolean parameters to the methods are removed (as it only tests using a Reader). Also, each function calls

StringReader reader = new StringReader(text); gen.writeString(reader, -1)

where it would previously call

gen.writeString(text); 

where text is the name of the String variable with the text to write.

Let me know if there is anything I can do to further test and/or improve this approach.

…ration ones and adapted to use Writers instead of Strings).

Modified one of the generators to manually allocate a buffer rather than call the same ctxt.allocXXXX method again in accordance with the instructions in the exception I was previously getting.
…eader. Changed the name of a test case file. Added failure test cases for reading from null reader, and for reading too little.
@uhhhh2
Copy link
Contributor Author

uhhhh2 commented Feb 10, 2017

I've edited the code so that it throws on a null reader or after reading too little. I've added some more unit tests (adapted from GeneratorFailTest and saved as GeneratorFailFromReaderTest) to test these types of failures.
Also, added a default implementation in GeneratorBase that simply reports the operation as "unsupported".

…s unsupported so that the new method doesn't have to be implemented in all formats right away.
@cowtowncoder
Copy link
Member

Looks good, and since I got CLA will merge for 2.9.

@cowtowncoder cowtowncoder merged commit 0711081 into FasterXML:master Feb 23, 2017
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.

2 participants