JSONWriter uses Appendable (v2)#259
Conversation
|
This is a much more straightforward change. The tests from the previous test cases apply. |
|
This looks simple enough. Your other pull request did have other good changes in it as well. Maybe if you find some good places to split the functional changes into separate pull requests it will be easier to get them approved. I would especially like to see consistency in how the JSONString interface is used within the library, so if you could open a pull request (or at least an issue) for that, I would appreciate it. It should note where the inconsistencies happen and the current way the interface is used at each point. We can then discuss the appropriate way that the interface should be handled from there. |
|
Looks good, will leave the pull request open for a few days for comments. |
3 participants
What problem does this code solve?
java.io.Writerto use ajava.lang.Appendableinstead. Situations whereAppendablecan be used where
Writercannot include theStringBuffer,StringBuilder, andCharBufferclasses.Risks
Low risk. This is modifying an API method to take an interface instead of a base class, which is generally a good practice. The positive unit test coverage is good. But JSONWriter is lacking coverage for most of the thrown exceptions, including in modified methods. OK to commit, but better unit test code coverage will be required before this code can be included in a release. See stleary/JSON-Java-unit-test#56.
Changes to the API?
The constructor for JSONWriter has changed from
WritertoAppendable.Will this require a new release?
Can be rolled into the next release, pending completion of the unit tests.
Should the documentation be updated?
Not required.
Does it break the unit tests?
No, the current tests run successfully. However, new unit tests have been added. See
stleary/JSON-Java-unit-test#53