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

JSONTokener should implement java.io.Closeable #718

Open
jtotht opened this issue Jan 28, 2023 · 9 comments
Open

JSONTokener should implement java.io.Closeable #718

jtotht opened this issue Jan 28, 2023 · 9 comments

Comments

@jtotht
Copy link

jtotht commented Jan 28, 2023

While JSONTokener(java.io.Reader) and JSONTokener(java.io.InputStream) constructors have explicit notes about JSONTokener not closing the reader/stream, I’d like to have it close the reader/stream. Implementing java.io.Closeable isn’t backward-incompatible (if the library user doesn’t explicitly close it, nothing happens), and for those who use try-with-resources construct, having to keep a separate reference for the reader/stream is a pain. Although try-with-resources itself is Java 7+, the java.io.Closeable interface is @since 1.5, so implementing the interface can be done without losing Java 6 compatibility.

@tsufiyan
Copy link

tsufiyan commented Apr 6, 2023

Shall I work on this? If yes, please assign

@haribabu-dev
Copy link
Contributor

Its completed & reviewed at commit e1eabc9

@jtotht
Copy link
Author

jtotht commented Apr 11, 2023

And reverted before getting merged. 🙁

@stleary
Copy link
Owner

stleary commented Sep 30, 2023

Closed due to lack of activity. Please post here if you think it should be reopened.

@stleary stleary closed this as completed Sep 30, 2023
@jtotht
Copy link
Author

jtotht commented Oct 1, 2023

Should I spam you with “what’s going on?” messages every few weeks? Doing so would just waste time of both of us. Lack of activity is not the same as lack of interest or usefulness. Please don’t close the issue until it gets implemented and released; especially not as “completed”, which is simply not true.

@stleary
Copy link
Owner

stleary commented Oct 1, 2023

@jtotht Thanks for the feedback. Historically, issues have been left open on this repo for an extended period, which has led to its own set of problems. I am making an effort to better manage the project by closing issues that are unlikely to be acted upon in the near term. The label "completed" that GitHub applies when an issue is closed with a comment is just GitHub behavior and does not necessarily mean that the requested feature or fix has been implemented. Given that the project is now primarily in maintenance mode, I am cautious about making major changes to the API, such as making JSONTokener implement Closeable. A change of this magnitude requires a compelling use case, given its potential impact on backward compatibility and API complexity. Please feel free to make that case if you wish.

@jtotht
Copy link
Author

jtotht commented Oct 3, 2023

I am making an effort to better manage the project by closing issues that are unlikely to be acted upon in the near term.

For things that may once be fixed/implemented, but unlikely to happen in the near term, you could introduce a label that makes this situation clear and allows hiding them in the issues list (example: hide in-progress issues) without closing the issue.

The label "completed" that GitHub applies when an issue is closed with a comment is just GitHub behavior and does not necessarily mean that the requested feature or fix has been implemented.

Recently (a few months ago, if I remember correctly) GitHub introduced a new feature allowing closing issues as “not planned”. With this new feature, the default of closing as “completed” does communicate that you believe that the requested feature has been implemented.

A change of this magnitude requires a compelling use case […]. Please feel free to make that case if you wish.

In my servlet application, I use JSON.org to parse JSON request bodies sent by the frontend. I do so by constructing JSONTokener objects using the return value of HttpServletRequest#getReader(). Since this reader needs to be closed once it’s no longer used, I have to pass both the Reader object and the JSONTokener object along (I need to pass them along because there are two methods – one for reading arrays and another one for reading objects –, but the extraction from request and error handling parts are common). Is this compelling enough considering the magnitude of this change? (Which isn’t that large IMO.)

given its potential impact on backward compatibility and API complexity

As I stated in the description, it’d be 100% backward-compatible: if one doesn’t call JSONTokener#close() and doesn’t use JSONTokener objects in try-with-resources constructs (neither of which is currently possible, so no existing users do so), nothing changes for them. It does make the API a bit more complex, but I believe that the benefits of being able to use the tokeners in try-with-resources constructs outweighs the costs of the API complexity increase.

@johnjaylward
Copy link
Contributor

It is not 100% backwards compatible. Adding the closable api makes integration with the android namespace implementation harder.

Code that uses the new iclosable would not be dropin to android, and that would need to be discussed.

@stleary
Copy link
Owner

stleary commented Oct 3, 2023

Re-opened per request

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

5 participants