commons-app / apps-android-commons

The Wikimedia Commons Android app allows users to upload pictures from their Android phone/tablet to Wikimedia Commons
https://commons-app.github.io/
Apache License 2.0
987 stars 1.18k forks source link

Replace GSON with Moshi for better kotlin support #3456

Open macgills opened 4 years ago

macgills commented 4 years ago

Gson does not play well with kotlin. Moshi would give us everything we need.

Would be a pretty sizeable change but worth it I would think.

maskaravivek commented 4 years ago

I just read up a bit about Moshi and it seems that it would indeed be a better choice than Gson given that we are now using Kotlin.

As of now just a handful of classes apart from tests uses Kotlin. This task can probably have a low priority for now.

madhurgupta10 commented 4 years ago

@maskaravivek I think we should proceed with this issue when at least 50% of the codebase is in Kotlin!

macgills commented 4 years ago

It is an impediment to writing good kotlin and honestly I found most of the gson implementation confusing. Why is everything marked expose?

@Documented
 @Retention(value=RUNTIME)
 @Target(value=FIELD)
public @interface Expose
An annotation that indicates this member should be exposed for JSON serialization or deserialization.
This annotation has no effect unless you build Gson with a GsonBuilder and invoke GsonBuilder.excludeFieldsWithoutExposeAnnotation() method

when the gson instance does not have excludeFieldsWithoutExposeAnnotation() set?

new GsonBuilder()
            .setDateFormat(DATE_FORMAT)
            .registerTypeHierarchyAdapter(Uri.class, new UriTypeAdapter().nullSafe())
            .registerTypeHierarchyAdapter(Namespace.class, new NamespaceTypeAdapter().nullSafe())
            .registerTypeAdapter(WikiSite.class, new WikiSiteTypeAdapter().nullSafe())
            .registerTypeAdapter(SharedPreferenceCookieManager.class, new CookieManagerTypeAdapter().nullSafe())
            .registerTypeAdapterFactory(new RequiredFieldsCheckOnReadTypeAdapterFactory())
            .registerTypeAdapterFactory(new PostProcessingTypeAdapter());

Why are we using @SerializedName when the field name is already correct?

To me a lot of this code is due for replacing and moshi works great with java and kotlin.

misaochan commented 4 years ago

I looked at https://github.com/square/moshi . Fairly reputable and looks relatively straightforward to use, and can be used with Java as well.

But if we do make the switch, I think we would need to commit to changing everything to Moshi, as leaving some parts in Gson would be untidy and confusing. So it may take a sizeable chunk of time to do it.

@macgills I think we can probably prioritize the structured-data related tasks and other smaller code quality fixes first, unless you would like to take this up in your spare time.