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

Remove Gson dependency and PureeLog interface #77

Merged

Conversation

VictorAlbertos
Copy link
Contributor

@VictorAlbertos VictorAlbertos commented Jul 22, 2020

This PR removes Gson dependency from Puree and replaces the types used for representing the serialized JSON, JsonObject and the JsonArray, by String and List<String> respectively. After doing this change, PureeLog was of no use so it has conveniently been removed.

The instrumentation tests, as such as the demo project and the docs, have been properly updated to reflect the latest changes

@VictorAlbertos
Copy link
Contributor Author

@k4zy this is the PR that I said I would open :)

@wraitho
Copy link

wraitho commented Jul 22, 2020

I'm not sure how many consumers this library has, but one thing to keep in mind that Victor's changes are breaking changes for the consumers of this library so if this is merged then Cookpad Global team and if the Japanese App uses it then you also need to add the Serializer. (and any other consumer)

Other than that, I think it looks good!

@VictorAlbertos
Copy link
Contributor Author

The main breaking change is the removal of the PureeLog interface: the clients of the library need now to stop implementing it. If this is not acceptable, I can keep the PureeLog interface as part of the API, even though it does nothing internally in the library.

The new requirement of supplying a serializer is easily solved in the case of Gson, as it is explained in the updated README:

public class PureeGsonSerializer implements PureeSerializer {
    private Gson gson = new Gson();

    @Override
    public String serialize(Object object) {
        return gson.toJson(object);
    }
}

From my point of view, the concrete implementation of the serializer should not be the responsibility of Puree. If you think that clients will benefit from shipping a PureeGsonSerializer, to avoid to have writing the above code, I can create another Gradle module called gson-serializer (as Puree core can't longer depend on Gson) and put there the PureeGsonSerializer.

Copy link

@wraitho wraitho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good :)

Copy link

@mbarta mbarta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

jsonObject.put("filter1", "foo");
return jsonObject.toString();
} catch (JSONException e) {
throw new RuntimeException(e);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for catching JsonException and re-throwing RuntimeException here? Also, the method's signature specifies it throws JsonException

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this is Java and that's a checked exception so it's forced me to re-trow with an unchecked exception 🤷‍♂️

@VictorAlbertos
Copy link
Contributor Author

Thanks for the reviews!

@VictorAlbertos VictorAlbertos merged commit 5ea26dd into cookpad:master Aug 7, 2020
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