Skip to content

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