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

Use the official Kotlin Serialization converter from Retrofit and remove references to Moshi #20

Merged
merged 6 commits into from
May 16, 2024

Conversation

cbeyls
Copy link
Contributor

@cbeyls cbeyls commented May 15, 2024

🎯 Goal

Remove a deprecated dependency and fix comments and documentation to reflect the actual libraries used in the project.

🛠 Implementation details

  • Use the official Kotlin Serialization converter from Retrofit instead of the deprecated library from Jake Wharton.
  • Remove unused versions.
  • Remove comments referencing Moshi.
  • Update the Readme file to mention Kotlin Serialization instead of Moshi.
  • Use "kotlinSerialization" as single dependency component instead of creating an empty "kotlin" subcomponent.
  • Declare OkHttp mock web server as a test dependency instead of a main dependency.
  • Rename some dependencies to be more explicit about what they are.
  • Replace deprecated call to project.buildDir in AndroidCompose convention plugin.

@cbeyls cbeyls requested a review from skydoves as a code owner May 15, 2024 18:40
@@ -70,10 +68,10 @@ hilt-compiler = { module = "com.google.dagger:hilt-compiler", version.ref = "hil
hilt-testing = { module = "com.google.dagger:hilt-android-testing", version.ref = "hilt" }
sandwich = { module = "com.github.skydoves:sandwich-retrofit", version.ref = "sandwich" }
retrofit = { module = "com.squareup.retrofit2:retrofit", version.ref = "retrofit" }
retrofit-kotlin-serialization = { group = "com.jakewharton.retrofit", name = "retrofit2-kotlinx-serialization-converter", version.ref = "retrofitKotlinxSerializationJson" }
okhttp-interceptor = { module = "com.squareup.okhttp3:logging-interceptor", version.ref = "okHttp" }
retrofit-kotlinSerialization = { module = "com.squareup.retrofit2:converter-kotlinx-serialization", version.ref = "retrofit" }
Copy link
Contributor

Choose a reason for hiding this comment

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

retrofit-kotlin-serialization is better in terms of clarity and per the naming convention

Copy link
Contributor Author

@cbeyls cbeyls May 15, 2024

Choose a reason for hiding this comment

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

There are different conventions, one of them is that the dash should only be used to separate sub-components and each component name should be camelCase. But I understand you prefer no camel case at all so I'll revert the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also note that the versions section currently use camel case for some components so it's kind of a mix.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes my bad is suppose to be retrofit-kotlinx-Serialization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I replace all kotlin-serialization references to kotlinx-serialization?

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be really good

@@ -108,4 +106,4 @@ hilt-plugin = { id = "com.google.dagger.hilt.android", version.ref = "hilt" }
baselineprofile = { id = "androidx.baselineprofile", version.ref = "baselineprofile" }

[bundles]
retrofitBundle = ["retrofit", "retrofit-kotlin-serialization", "okhttp-interceptor", "okhttp-mockserver"]
retrofit = ["retrofit", "retrofit-kotlinSerialization", "okhttp-loggingInterceptor"]
Copy link
Contributor

Choose a reason for hiding this comment

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

retofitBundle makes it easier to understand at a glance which dependencies are grouped together.

Copy link
Contributor

@FreedomChuks FreedomChuks May 15, 2024

Choose a reason for hiding this comment

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

and correct as per naming convention for bundles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it was redundant because it's already part of the bundle namespace:
bundle.retrofitBundle

Copy link
Contributor

Choose a reason for hiding this comment

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

for code readability is easy to know that it bundled

@cbeyls
Copy link
Contributor Author

cbeyls commented May 15, 2024

For some reason Spotless believes that @Composable function names should start with a lowercase letter and reports all sorts of errors in classes this PR didn't touch.

@FreedomChuks
Copy link
Contributor

For some reason Spotless believes that @Composable function names should start with a lowercase letter and reports all sorts of errors in classes this PR didn't touch.

i have raised a PR to fix that once it get merged just call ./gradlew spotlessApply @cbeyls

@FreedomChuks
Copy link
Contributor

@cbeyls rebase pull and run ./gradlew spotlessApply

…pendency

- rename some dependencies to be more explicit about what they are.
- replace deprecated call to project.buildDir
…nd use dashes instead of camel case for dependency component names.
@cbeyls cbeyls force-pushed the chore/update_kotlinx_serialization branch from 33d31d1 to e9fb32d Compare May 16, 2024 10:37
@cbeyls cbeyls requested a review from FreedomChuks May 16, 2024 10:41
Copy link
Owner

@skydoves skydoves left a comment

Choose a reason for hiding this comment

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

This is fantastic!! Nicely done, @cbeyls and @FreedomChuks 🙌 🙌
Thank you for your valuable contribution.

@skydoves skydoves merged commit 6dce6d0 into skydoves:main May 16, 2024
2 checks passed
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