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

Moshi #72

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

Moshi #72

wants to merge 1 commit into from

Conversation

epicadk
Copy link
Contributor

@epicadk epicadk commented Nov 25, 2020

Description

Added Moshi converter Factory fixes #68

Please ensure the following and put a check accordingly

  • My code doesn't break/affect any unrelated part of the codebase.

  • Ran ./gradlew checkstyle

  • Squashed multiple commits (if irrelevant) into one commit

@epicadk epicadk changed the base branch from dev to backend_int November 25, 2020 08:46
@epicadk
Copy link
Contributor Author

epicadk commented Nov 25, 2020

Moshi Requires that naming convention to work faster that's why the checkstyle test is failing. I don't see a problem with it as we are using the getters and setters anyways

https://stackoverflow.com/questions/4023185/how-to-disable-a-particular-checkstyle-rule-for-a-particular-line-of-code

@@ -57,6 +57,8 @@ dependencies {
// Retrofit
implementation "com.squareup.retrofit2:retrofit:$rootProject.retrofit_version"
implementation "com.squareup.retrofit2:converter-gson:$rootProject.retrofit_version"
implementation "com.squareup.moshi:moshi:1.11.0"
Copy link
Owner

Choose a reason for hiding this comment

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

Please use the usual dependency version specification style that has been used for others

return new OkHttpClient()
.newBuilder()
.addInterceptor(new ZomatoApiInterceptor())
.addInterceptor(logging)
.readTimeout(5, TimeUnit.SECONDS)
Copy link
Owner

Choose a reason for hiding this comment

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

@epicadk we are not expecting a huge load on the server so I believe we can keep lower socket time out values. But still, I believe the connection time out value can be set as low as 5 seconds but the socket timeout can be dragged a little higher like 10 seconds.

What was your reasoning behind setting this as 5 seconds?

@@ -8,12 +8,12 @@
@Expose
private int rating;
@Expose
private int zomatoResId;
private int zomato_res_id;
Copy link
Owner

Choose a reason for hiding this comment

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

Camel case!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah moshi requires snake case.

public class RefreshResponse {
String authToken;
String auth_token;
Copy link
Owner

Choose a reason for hiding this comment

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

Here also

private String authToken;
@Expose
private String refreshToken;
private String auth_token;
Copy link
Owner

Choose a reason for hiding this comment

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

You are not following the camel case style of writing and because of this build tests are failing. Please fix this

@epicadk epicadk requested a review from devansh-299 January 10, 2021 19:02
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.

2 participants