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

made JSONArray Iterable #130

Closed
wants to merge 3 commits into from
Closed

made JSONArray Iterable #130

wants to merge 3 commits into from

Conversation

Haringat
Copy link

@Haringat Haringat commented Jun 4, 2015

this gives you the possibility to use them in Java's for-each loops
Edit: I also moved the source into a subfolder with the package to make it easier to clone and edit the project

@johnjaylward
Copy link
Contributor

I'm not a fan of the default implementation you have to toJSONString. If you break that out into a separate pull request it would be better. I do like that you updated the JSONException to take a throwable argument as that's one of the first things I changed myself.

I'm also not sure what changed in most of those files. If it was whitespace changes, you should not have committed those. Try to keep file formatting as close to the projects formatting as possible so that multiple committers aren't fighting over whitespace and line wrapping issues.

@johnjaylward
Copy link
Contributor

oh, nevermind, I see the changes. Please don't rename the files. Other pull requests have done this and been rejected.

@Haringat
Copy link
Author

Haringat commented Jun 5, 2015

Well if one doesn't like the default implementation he is always free to override it (he will have to anyway especially if not all fields should be overriden). I just made something which I thought to be the most straightforward implementation for this.
And concerning the moving of the source files I have 3 arguments for this:

  1. It keeps the project root clean
  2. As I mentioned earlier: It makes it easier to clone and compile the project.
  3. I'm currently working on introducing gradle to the project and for this to work the files had to be mved. (and I found out that I had to move them again but that is not committed yet)

@stleary stleary mentioned this pull request Jun 5, 2015
@stleary
Copy link
Owner

stleary commented Jun 5, 2015

Thank you for the iterable contribution. Pending comments, it will be merged in a few days on #132. I notice that using an iterator can require a lot of casting that was previously managed by the existing API, but I think some will find it helpful. File moves have been declined numerous times in previous pull requests. At this point, I don't see a compelling use case for changing JSONString.

@stleary stleary closed this Jun 5, 2015
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