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

Error message position fixes #352

Merged
merged 3 commits into from
Jul 6, 2017

Conversation

johnjaylward
Copy link
Contributor

@johnjaylward johnjaylward commented Jun 23, 2017

Key Changes:

  • Fix error message position information to be accurate

What problem does this code solve?
Corrects incorrect position information when generating syntax errors in tokeners.

Risks
None. The current position information is wildly off in some cases. It's not helpful to anyone trying to debug a failing document.

Changes to the API?
none.

Will this require a new release?
Yes, this is a bug fix that should probably be corrected as soon as possible

Should the documentation be updated?
No

Does it break the unit tests?
Yes. The current position information in the XML error messages is wrong/inaccurate. The changes here in regards to the JSONTokener.more function have fixed many of the position problems in the old code. Some other pieces of code needed to be updated as well.

See the corrected tests here: stleary/JSON-Java-unit-test#74

Was any code refactored in this commit?
Yes. The JSONToken.more function was changed to use the mark/reset support in the stream reader instead of stepping forward and backward. This gives us faster checking on more() as well as more accurate position information as we aren't moving our index around every time we want to see if there is more information in the stream.

Also, a new field characterPreviousLine was added to track the previous line's character count in order to preserve character information when stepping back at a new line mark.

Review status
ACCEPTED, starting 3 day comment window.

@johnjaylward
Copy link
Contributor Author

@stleary any chance to review this yet? No rush (I know it's holiday time for US and Canada), just wanted to make sure it wasn't missed or forgotten.

@stleary
Copy link
Owner

stleary commented Jul 3, 2017

Thanks for the reminder, holidays and weekends are actually the source for a lot of my free time :) Agree the bug fix should be committed. Any chance you can separate that code from the formatting changes?
Per the FAQ, code formatting changes are normally not accepted. I was trying to make an exception in this case, but as you noted, no progress has been made.

@johnjaylward
Copy link
Contributor Author

sure, I think I can just revert that one commit

@johnjaylward
Copy link
Contributor Author

the revert was clean. Should just be these changes now.

@johnjaylward johnjaylward force-pushed the ErrorMessagePositionFixes branch from 98e07e1 to 16baa32 Compare July 3, 2017 17:03
@stleary
Copy link
Owner

stleary commented Jul 3, 2017

Thanks for backing out the reformatting changes.

@johnjaylward
Copy link
Contributor Author

no problem. If possible though, I would like the code to be more consistent. Right now the mixed spaces/tabs and the inconsistent indentation make it harder to contribute. If we could settle on something then reformat to match, that would be very helpful.

@johnjaylward
Copy link
Contributor Author

@stleary also, I'm wondering if I should back out some of the recent changes I made that make it even harder for Android compatibility before doing a release. Not all the change in #350 , but specifically the ones for entrySet and maybe some others I can't think of off the top of my head. Or should we just wait for #350 to get tested and merged before worrying about android?

It does looks like my change to the Android JSONException class are getting added to Android Master soon, but still no guarantee that it will be released in a timely fashion for Android Developers to use.

@stleary
Copy link
Owner

stleary commented Jul 3, 2017

Sure! How does indent 4 spaces, no tabs sound? A commit that contains only formatting changes for one or more files, and passes unit tests, would be fine.

@stleary
Copy link
Owner

stleary commented Jul 3, 2017

It seems to me that an actual Android developer who uses JSON-Java is needed to Beta test #350, and then recommend approval or further changes.

@stleary stleary merged commit 5024f2d into stleary:master Jul 6, 2017
@johnjaylward johnjaylward deleted the ErrorMessagePositionFixes branch July 7, 2017 16:30
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.

2 participants