Skip to content
This repository has been archived by the owner on Sep 14, 2021. It is now read-only.

Replaced Json with Gson to rid of its dependency #39

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

acwwat
Copy link
Contributor

@acwwat acwwat commented Jan 23, 2016

Per issue #33, I have replaced Json usages with that of Gson. However I have some concerns:

  1. Since JSONException references have to be removed, it had to be removed from numerous method signatures.
  2. Gson throws RuntimExceptions instead of Exceptions on object type incompatibility. This changes some exception handling behaviour vs. Json which uses JSONException. The updated code will just let RuntimeExceptions trickle up.
  3. Gson API is not fluent and requires more chained calls.

Please review and see if these are OK when reviewing the pull request. If it's too much risk we can leave the original code alone. Thanks.

@liujin-google
Copy link
Contributor

Thank you Anthony for the pull request. Reducing dependencies does make sense, but I share the same concern that probably too many changes are involved. Also we need to guarantee the backward compatibility with existing developers' code using the Identity Toolkit library. We are thinking of a new library version and will put the Json/Gson redundancy into consideration. Due to these reasons, I incline to close the pull request.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants