-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Updates for supporting the Android API #350
Conversation
Will this change allow Android users to do XML parsing? |
yeah, it should allow XML parsing. I had a few errors, I'm fixing them now. |
ok, I fixed the errors in the code here, and also needed to update the tests to have correct position information in the syntax errors. The implementation on Master has bad position information as it's expected cases. |
@AppWerft @quelgar @AvinashChowdary @powerqian |
This should be ready for live testing now. I believe I corrected all the wrong positions in any error messages. As noted in the main comment, if used with Android, my changes are expecting you to NOT use the classes in this library that Android already provides. My changes should allow you to use the XMLTokener and XML classes directly with the Android org.json namespace. |
9b1b551
to
841e2a3
Compare
I rebased this pull request to depend on #352 in case we want to fix the error message position errors first. |
841e2a3
to
1736a60
Compare
rebased back to master now that Tokener has been updated |
@johnjaylward do you still want to proceed with this? Do you recommend releasing an Android-only jar? |
Yeah, I think it could still be useful
…On Oct 15, 2017 11:12, "Sean Leary" ***@***.***> wrote:
@johnjaylward <https://github.com/johnjaylward> do you still want to
proceed with this? Do you recommend releasing an Android-only jar?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#350 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAXa18ZbwIV4CB6Mw5paOZefyLoTY5kPks5ssiDAgaJpZM4OBMD6>
.
|
I will reach out to @BGehrels for a checkpoint release prior to the Android fix. Thought I had already opened an issue. |
@johnjaylward 20171018 is released and published on the Maven repo. OK to proceed if you want. |
Updated to Master. This should be compatible with Android 8.0 (API level 26) with no issues as long as the overlapping classes are removed. It will be mostly compatible with Android API<=25 as long as no exceptions are thrown that include the "cause" exception. in those cases a MethodNotFoundException is thrown instead of the JSONException. |
Accepted, starting 3 day window. |
@johnjaylward can you recommend how an Android dev might go about using these changes? What to include in the jar, any issues they should be aware of, etc? |
Android Compatibility:
How to use as a JAR:
How to use from source
This will then use the built in Android JSON implementation and allow you to use JSONPointer, the XML tools, etc. |
Thanks! |
Key Changes:
If this is accepted, we should be able to release a secondary JAR, or Android users can roll their own JAR that does not include the following files:
This should prevent any class loader issues between the 2 projects.
What problem does this code solve?
This removes API incompatibilities from our implementation to better match whats available in the Android implementation.
Risks
Medium. One public API in XMLTokener was changed to not have a return value. The superclass of XMLTokener is JSONTokener which is also included in Android. The Android version of JSONTokener has a public method of the same name, but with a void return. In order to have the XMLTokener extend the Android version of JSONTokener, the return values should match.
The risk here is delegated as "Medium" because it's unlikely that anyone is actually calling the method in question. it's very much an internal method that should probably never have been made public in the first place.
Also to note, the Android version of JSONException has the following 2 incompatibilities:
In order to better match the Android version of JSONException I updated some method signatures in JSONPointer to include the exception cases.
I left comments in the code where incompatible JSONException instances are made. I did not change our implementation of JSONException.
I did submit a change request to Android to extend the JSONException class to have more constructors to match our API. The constructors may be a non-issue on future versions of Android. See https://android-review.googlesource.com/#/c/420159/
This change has been approved and merged into Android Master and should be part of the next android API release.
Changes to the API?
Noted in the Risks section.
Will this require a new release?
No, can be rolled into the next release
Should the documentation be updated?
No
Does it break the unit tests?
No. All unit tests should pass.
Was any code refactored in this commit?
Yes
Review status
ACCEPTED
Starting 3 day window for comments.