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

JSONWriter uses Appendable (v2) #259

Merged
merged 1 commit into from
Aug 11, 2016
Merged

Conversation

run2000
Copy link
Contributor

@run2000 run2000 commented Aug 8, 2016

What problem does this code solve?

  • Changes methods that take a java.io.Writer to use a
    java.lang.Appendable instead. Situations where Appendable
    can be used where Writer cannot include the StringBuffer,
    StringBuilder, and CharBuffer classes.

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 Writer to Appendable.

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

@run2000
Copy link
Contributor Author

run2000 commented Aug 8, 2016

This is a much more straightforward change. The tests from the previous test cases apply.

@johnjaylward
Copy link
Contributor

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.

@stleary
Copy link
Owner

stleary commented Aug 9, 2016

Looks good, will leave the pull request open for a few days for comments.

@stleary stleary merged commit 4e8e24d into stleary:master Aug 11, 2016
datable-ueshima pushed a commit to datable-ueshima/JSON-java that referenced this pull request Dec 5, 2022
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