Closed sriramr98 closed 6 years ago
We don't want to just generate code and remove lombok. Adding boilerplate is not an option. A solution should be there. So in whichever class lombok was used except maybe for builder, should have kotlin specific methods to reduce boilerplate instead
Yeah, I agree. I just thought that they should have separate PR's because they have two separate open issues. I made the Kotlin changes already and was gonna give a second PR after this was merged.
I think there'd be no point to add boilerplate just to remove it. Because this commit will be dirty history then
Alright Ill integrate Kotlin now
Hi @sriramr98!
It looks like one or more of your builds have failed. I've added the relevant info below to save you some time.
failed
Opposite behaviour is seen. The classes which logically don't need equals and hashCode have those and these classes can be converted to data classes as well. The model classes converted to kotlin don't have equals and hashCode and they require it
Thank you. I noticed these and I have already started making the changes. I'll commit the changes soon.
On Wed 14 Mar, 2018, 12:28 PM Areeb Jamal, notifications@github.com wrote:
@iamareebjamal requested changes on this pull request.
In android/app/src/main/java/org/fossasia/openevent/config/Config.java https://github.com/fossasia/open-event-android/pull/2373#discussion_r174363724 :
- if (!(o instanceof Config)) return false;
- final Config other = (Config) o;
- if (!other.canEqual((Object) this)) return false;
- final Object this$apiLink = this.getApiLink();
- final Object other$apiLink = other.getApiLink();
- if (this$apiLink == null ? other$apiLink != null : !this$apiLink.equals(other$apiLink))
- return false;
- final Object this$email = this.getEmail();
- final Object other$email = other.getEmail();
- if (this$email == null ? other$email != null : !this$email.equals(other$email))
- return false;
- final Object this$appName = this.getAppName();
- final Object other$appName = other.getAppName();
- if (this$appName == null ? other$appName != null : !this$appName.equals(other$appName))
- return false;
- if (this.getAuthEnabled() != other.getAuthEnabled()) return false;
Remove equals and hashCode unless in model classes
In android/app/src/main/java/org/fossasia/openevent/config/Config.java https://github.com/fossasia/open-event-android/pull/2373#discussion_r174363908 :
- public int hashCode() {
- final int PRIME = 59;
- int result = 1;
- final Object $apiLink = this.getApiLink();
- result = result * PRIME + ($apiLink == null ? 43 : $apiLink.hashCode());
- final Object $email = this.getEmail();
- result = result * PRIME + ($email == null ? 43 : $email.hashCode());
- final Object $appName = this.getAppName();
- result = result * PRIME + ($appName == null ? 43 : $appName.hashCode());
- result = result * PRIME + (this.getAuthEnabled() ? 79 : 97);
- return result;
- }
- protected boolean canEqual(Object other) {
- return other instanceof Config;
- }
Not required
In android/app/src/main/java/org/fossasia/openevent/config/Config.java https://github.com/fossasia/open-event-android/pull/2373#discussion_r174363959 :
public class Config { private final String apiLink; private final String email; private final String appName; private final boolean authEnabled; +
- Config(String apiLink, String email, String appName, boolean authEnabled) {
Should be private or public
In android/app/src/main/java/org/fossasia/openevent/core/auth/LoginActivityViewModel.java https://github.com/fossasia/open-event-android/pull/2373#discussion_r174364248 :
- return this.response;
- }
- public String getAccessToken() {
- return this.accessToken;
- }
- public boolean equals(Object o) {
- if (o == this) return true;
- if (!(o instanceof LoginResponse)) return false;
- final LoginResponse other = (LoginResponse) o;
- if (!other.canEqual((Object) this)) return false;
- if (this.getResponse() != other.getResponse()) return false;
- final Object this$accessToken = this.getAccessToken();
- final Object other$accessToken = other.getAccessToken();
- if (this$accessToken == null ? other$accessToken != null : !this$accessToken.equals(other$accessToken))
For classes which aren't models, there is no need for equals and hash code or you can take this class and create a kotlin data class in the same package
In android/app/src/main/java/org/fossasia/openevent/core/auth/model/User.kt https://github.com/fossasia/open-event-android/pull/2373#discussion_r174364637 :
+import com.fasterxml.jackson.databind.PropertyNamingStrategy +import com.fasterxml.jackson.databind.annotation.JsonNaming +import com.github.jasminb.jsonapi.IntegerIdHandler +import com.github.jasminb.jsonapi.annotations.Id +import com.github.jasminb.jsonapi.annotations.Type + +import io.realm.RealmObject +import io.realm.annotations.Ignore +import io.realm.annotations.PrimaryKey + +@Type("user") +@JsonNaming(PropertyNamingStrategy.KebabCaseStrategy::class) +open class User(
- @PrimaryKey
- @Id(IntegerIdHandler::class)
- var id: Int = 0,
Make nullable and default to null
In android/app/src/main/java/org/fossasia/openevent/core/auth/model/User.kt https://github.com/fossasia/open-event-android/pull/2373#discussion_r174364779 :
- private var isSuperAdmin: Boolean = false
- private var createdAt: String? = null
- private var lastAccessedAt: String? = null
- private var contact: String? = null
- private var deletedAt: String? = null
- private var details: String? = null
- private var isVerified: Boolean = false
- private var thumbnailImageUrl: String? = null
- private var iconImageUrl: String? = null
- private var smallImageUrl: String? = null
- private var avatarUrl: String? = null
- private var facebookUrl: String? = null
- private var twitterUrl: String? = null
- private var instagramUrl: String? = null
- private var googlePlusUrl: String? = null
- private var originalImageUrl: String? = null
Massive duplication, not recommended. In model classes, don't create builders.
In android/app/src/main/java/org/fossasia/openevent/core/bookmark/BookmarkStatus.java https://github.com/fossasia/open-event-android/pull/2373#discussion_r174364868 :
public class BookmarkStatus {
private int storedColor; private int sessionId; private Status actionCode;
- public BookmarkStatus(int storedColor, int sessionId, Status actionCode) {
Can be converted to data class
In android/app/src/main/java/org/fossasia/openevent/core/feed/facebook/FacebookFeedFragmentViewModel.java https://github.com/fossasia/open-event-android/pull/2373#discussion_r174364913 :
public static class PostsResponse {
private final int response; private final Feed feed; +
- public PostsResponse(int response, Feed feed) {
Can be converted to data class
In android/app/src/main/java/org/fossasia/openevent/core/feed/facebook/api/CommentItem.kt https://github.com/fossasia/open-event-android/pull/2373#discussion_r174365118 :
+ +import android.os.Parcel +import android.os.Parcelable + +import com.fasterxml.jackson.annotation.JsonIgnoreProperties +import com.fasterxml.jackson.databind.PropertyNamingStrategy +import com.fasterxml.jackson.databind.annotation.JsonNaming + +@JsonIgnoreProperties(ignoreUnknown = true) +@JsonNaming(PropertyNamingStrategy.SnakeCaseStrategy::class) +data class CommentItem(
- val createdTime: String? = null,
- val from: Commenter? = null,
- val message: String? = null,
- val id: String? = null +) : Parcelable {
Use kotlin extension @Parcelize
In android/app/src/main/java/org/fossasia/openevent/core/feed/facebook/api/FacebookPageId.kt https://github.com/fossasia/open-event-android/pull/2373#discussion_r174365169 :
@@ -0,0 +1,6 @@ +package org.fossasia.openevent.core.feed.facebook.api + +data class FacebookPageId (
- var name: String? = null,
- var id: String? = null
- )
Indentation fault
In android/app/src/main/java/org/fossasia/openevent/core/feed/facebook/api/FeedItem.kt https://github.com/fossasia/open-event-android/pull/2373#discussion_r174365196 :
+package org.fossasia.openevent.core.feed.facebook.api + +import com.fasterxml.jackson.annotation.JsonIgnoreProperties +import com.fasterxml.jackson.databind.PropertyNamingStrategy +import com.fasterxml.jackson.databind.annotation.JsonNaming + +@JsonIgnoreProperties(ignoreUnknown = true) +@JsonNaming(PropertyNamingStrategy.SnakeCaseStrategy::class) +data class FeedItem (
- var id: String? = null,
- var message: String? = null,
- var createdTime: String? = null,
- var comments: Comments? = null,
- var fullPicture: String? = null,
- var link: String? = null
- )
Indentation fault
In android/app/src/main/java/org/fossasia/openevent/core/feed/twitter/TwitterFeedFragmentViewModel.java https://github.com/fossasia/open-event-android/pull/2373#discussion_r174365234 :
public static class PostsResponse {
private final int response; private final TwitterFeed twitterFeed; +
- public PostsResponse(int response, TwitterFeed twitterFeed) {
Can be converted to data class
In android/app/src/main/java/org/fossasia/openevent/core/feed/twitter/api/TwitterFeed.kt https://github.com/fossasia/open-event-android/pull/2373#discussion_r174365288 :
@@ -0,0 +1,5 @@ +package org.fossasia.openevent.core.feed.twitter.api + +import java.util.ArrayList + +data class TwitterFeed ( var statuses: ArrayList
? = null ) Don't use concrete types, use List
In android/app/src/main/java/org/fossasia/openevent/core/feed/facebook/api/Feed.kt https://github.com/fossasia/open-event-android/pull/2373#discussion_r174365335 :
@@ -0,0 +1,5 @@ +package org.fossasia.openevent.core.feed.facebook.api + +import java.util.ArrayList + +data class Feed ( var data: ArrayList
? = null ) Don't use concrete types, use List
In android/app/src/main/java/org/fossasia/openevent/core/feed/twitter/api/TwitterFeedItem.kt https://github.com/fossasia/open-event-android/pull/2373#discussion_r174365432 :
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties +import com.fasterxml.jackson.databind.PropertyNamingStrategy +import com.fasterxml.jackson.databind.annotation.JsonNaming + +import java.util.ArrayList + +@JsonIgnoreProperties(ignoreUnknown = true) +@JsonNaming(PropertyNamingStrategy.SnakeCaseStrategy::class) +data class TwitterFeedItem (
- var link: String? = null,
- var hashTags: ArrayList
? = null, - var links: ArrayList
? = null, - var images: ArrayList
? = null, - var text: String? = null,
- var createdAt: String? = null
- )
Indentation fault
In android/app/src/main/java/org/fossasia/openevent/core/feed/twitter/api/TwitterFeedItem.kt https://github.com/fossasia/open-event-android/pull/2373#discussion_r174365447 :
@@ -0,0 +1,18 @@ +package org.fossasia.openevent.core.feed.twitter.api + +import com.fasterxml.jackson.annotation.JsonIgnoreProperties +import com.fasterxml.jackson.databind.PropertyNamingStrategy +import com.fasterxml.jackson.databind.annotation.JsonNaming + +import java.util.ArrayList + +@JsonIgnoreProperties(ignoreUnknown = true) +@JsonNaming(PropertyNamingStrategy.SnakeCaseStrategy::class) +data class TwitterFeedItem (
- var link: String? = null,
- var hashTags: ArrayList
? = null, - var links: ArrayList
? = null, - var images: ArrayList
? = null, Don't use concrete types, use List
In android/app/src/main/java/org/fossasia/openevent/core/schedule/ScheduleFragmentViewModel.java https://github.com/fossasia/open-event-android/pull/2373#discussion_r174365501 :
public static class EventDateStrings {
private final String formattedDate; private final String date; +
- public EventDateStrings(String formattedDate, String date) {
Can be converted to data class
In android/app/src/main/java/org/fossasia/openevent/data/Event.kt https://github.com/fossasia/open-event-android/pull/2373#discussion_r174365583 :
+import com.github.jasminb.jsonapi.annotations.Type + +import org.fossasia.openevent.data.extras.Copyright +import org.fossasia.openevent.data.extras.SocialLink +import org.fossasia.openevent.data.extras.SpeakersCall + +import io.realm.RealmList +import io.realm.RealmObject +import io.realm.annotations.PrimaryKey + +@Type("event") +@JsonNaming(PropertyNamingStrategy.KebabCaseStrategy::class) +open class Event(
- @PrimaryKey
- @Id(IntegerIdHandler::class)
- var id: Int = 0,
Nullable and default to null
In android/app/src/main/java/org/fossasia/openevent/data/FAQ.kt https://github.com/fossasia/open-event-android/pull/2373#discussion_r174365649 :
@@ -0,0 +1,27 @@ +package org.fossasia.openevent.data + +import com.fasterxml.jackson.databind.PropertyNamingStrategy +import com.fasterxml.jackson.databind.annotation.JsonNaming +import com.github.jasminb.jsonapi.IntegerIdHandler +import com.github.jasminb.jsonapi.annotations.Id +import com.github.jasminb.jsonapi.annotations.Type + +import io.realm.RealmObject +import io.realm.annotations.PrimaryKey + +/**
- Created by mayank on 4/2/18.
- */
Please remove the author comment
In android/app/src/main/java/org/fossasia/openevent/data/FAQ.kt https://github.com/fossasia/open-event-android/pull/2373#discussion_r174365684 :
+import com.github.jasminb.jsonapi.IntegerIdHandler +import com.github.jasminb.jsonapi.annotations.Id +import com.github.jasminb.jsonapi.annotations.Type + +import io.realm.RealmObject +import io.realm.annotations.PrimaryKey + +/**
- Created by mayank on 4/2/18.
- */ +@Type("faq") +@JsonNaming(PropertyNamingStrategy.KebabCaseStrategy::class) +open class FAQ(
- @PrimaryKey
- @Id(IntegerIdHandler::class)
- var id: Int = 0,
Make nullable and default to null
In android/app/src/main/java/org/fossasia/openevent/data/Microlocation.kt https://github.com/fossasia/open-event-android/pull/2373#discussion_r174365742 :
+ +import com.fasterxml.jackson.databind.PropertyNamingStrategy +import com.fasterxml.jackson.databind.annotation.JsonNaming +import com.github.jasminb.jsonapi.IntegerIdHandler +import com.github.jasminb.jsonapi.annotations.Id +import com.github.jasminb.jsonapi.annotations.Type + +import io.realm.RealmObject +import io.realm.annotations.PrimaryKey + +@Type("microlocation") +@JsonNaming(PropertyNamingStrategy.KebabCaseStrategy::class) +open class Microlocation(
- @PrimaryKey
- @Id(IntegerIdHandler::class)
- var id: Int = 0,
Nullable and default to null
In android/app/src/main/java/org/fossasia/openevent/data/Microlocation.kt https://github.com/fossasia/open-event-android/pull/2373#discussion_r174365777 :
+import com.github.jasminb.jsonapi.IntegerIdHandler +import com.github.jasminb.jsonapi.annotations.Id +import com.github.jasminb.jsonapi.annotations.Type + +import io.realm.RealmObject +import io.realm.annotations.PrimaryKey + +@Type("microlocation") +@JsonNaming(PropertyNamingStrategy.KebabCaseStrategy::class) +open class Microlocation(
- @PrimaryKey
- @Id(IntegerIdHandler::class)
- var id: Int = 0,
- var name: String? = null,
- var latitude: Float = 0.toFloat(),
- var longitude: Float = 0.toFloat(),
Nullable and default to null
In android/app/src/main/java/org/fossasia/openevent/data/Microlocation.kt https://github.com/fossasia/open-event-android/pull/2373#discussion_r174365857 :
+import com.github.jasminb.jsonapi.IntegerIdHandler +import com.github.jasminb.jsonapi.annotations.Id +import com.github.jasminb.jsonapi.annotations.Type + +import io.realm.RealmObject +import io.realm.annotations.PrimaryKey + +@Type("microlocation") +@JsonNaming(PropertyNamingStrategy.KebabCaseStrategy::class) +open class Microlocation(
- @PrimaryKey
- @Id(IntegerIdHandler::class)
- var id: Int = 0,
- var name: String? = null,
- var latitude: Float = 0.toFloat(),
- var longitude: Float = 0.toFloat(),
Nullable and default to null is the direct Java equivalent and should be used everywhere
In android/app/src/main/java/org/fossasia/openevent/data/Microlocation.kt https://github.com/fossasia/open-event-android/pull/2373#discussion_r174366115 :
+import com.github.jasminb.jsonapi.IntegerIdHandler +import com.github.jasminb.jsonapi.annotations.Id +import com.github.jasminb.jsonapi.annotations.Type + +import io.realm.RealmObject +import io.realm.annotations.PrimaryKey + +@Type("microlocation") +@JsonNaming(PropertyNamingStrategy.KebabCaseStrategy::class) +open class Microlocation(
- @PrimaryKey
- @Id(IntegerIdHandler::class)
- var id: Int = 0,
- var name: String? = null,
- var latitude: Float = 0.toFloat(),
- var longitude: Float = 0.toFloat(),
If the latitude and longitude are not set, they should be null and not point to [https://en.wikipedia.org/wiki/Null_Island](Null Island). Similar logical errors will creep in if you'll assign default values to properties
Also, kotlin will throw error while parsing if JSON contains null
In android/app/src/main/java/org/fossasia/openevent/data/Microlocation.kt https://github.com/fossasia/open-event-android/pull/2373#discussion_r174366219 :
+open class Microlocation(
- @PrimaryKey
- @Id(IntegerIdHandler::class)
- var id: Int = 0,
- var name: String? = null,
- var latitude: Float = 0.toFloat(),
- var longitude: Float = 0.toFloat(),
- var floor: Int = 0,
- var room: String? = null
+) : RealmObject() {
- constructor(id: Int, name: String) : this() {
- this.id = id
- this.name = name
- }
Remove extra constructor, we will used named properties in kotlin and in Java. we will manually set the id and name when creating objects
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/fossasia/open-event-android/pull/2373#pullrequestreview-103703899, or mute the thread https://github.com/notifications/unsubscribe-auth/AUTzqSyz8qE1Ci-H_E0N1I76ZSYrXsh1ks5teL-egaJpZM4SmqRB .
Also, use @JvmStatic
on companion object fields
Changed as requested. If the PR is accepted, close issue #2357 too as this PR fixes that issue too.
Hi @sriramr98!
It looks like one or more of your builds have failed. I've added the relevant info below to save you some time.
failed
I've added comments. Some issues added previously haven't been taken care of. Also the build is failing. As soon as the build and codacy warnings are fixed with the issues raised, we are good to go
Hi @sriramr98!
It looks like one or more of your builds have failed. I've added the relevant info below to save you some time.
failed
Hi @sriramr98!
It looks like one or more of your builds have failed. I've added the relevant info below to save you some time.
failed
@sriramr98 The tests have to be updated, hence the build is failing
:app:kaptFdroidDebugUnitTestKotlin<==========---> 82% EXECUTING [6m 12s]> :app:compileFdroidDebugUnitTestKotlin<==========---> 84% EXECUTING [6m 12s]> :app:compileFdroidDebugUnitTestJavaWithJavac> Task :app:compileFdroidDebugUnitTestJavaWithJavac
/home/travis/build/fossasia/open-event-android/android/app/src/test/java/org/fossasia/openevent/common/utils/JSONDeserializationTest.java:40: error: reference to assertEquals is ambiguous
assertEquals(6, event.getId());
^
both method assertEquals(long,long) in Assert and method assertEquals(Object,Object) in Assert match
/home/travis/build/fossasia/open-event-android/android/app/src/test/java/org/fossasia/openevent/common/utils/JSONDeserializationTest.java:41: error: cannot find symbol
assertTrue(event.getIsSessionsSpeakersEnabled());
^
symbol: method getIsSessionsSpeakersEnabled()
location: variable event of type Event
/home/travis/build/fossasia/open-event-android/android/app/src/test/java/org/fossasia/openevent/common/utils/JSONDeserializationTest.java:73: error: cannot find symbol
assertFalse(sessions.get(0).getIsMailSent());
^
symbol: method getIsMailSent()
location: class Session
/home/travis/build/fossasia/open-event-android/android/app/src/test/java/org/fossasia/openevent/common/utils/JSONDeserializationTest.java:88: error: cannot find symbol
assertFalse(speakers.get(0).getIsFeatured());
^
symbol: method getIsFeatured()
location: class Speaker
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
4 errors
Also, refactor your branch to latest dev branch or there'll be several conflicts as they are already
It is in the dev branch and I did a git pull. It says that the branch is already up to date
You did git pull origin development
You have to do git pull upstream development
Oh, looks like you have done all changes in development branch itself rather than creating a new branch. BIG MISTAKE
DO NOT DO GIT PULL Normally, I'd ask to change to a new branch by moving commits but that will make this PR obsolete. So First remove all the unneeded changes so that commits don't have 186 files. There were about ~40 files changed. So remove all the unneeded changes and when you are sure that only required files are changed. Commit this change. Now you should have 12 commits in total. Squash all your commits into 1 so that you don't have to deal with rebasing 12 commits and interleaved commits from dev branch in between
*I would even recommend to undo your last commit of breaking stuff into modules as you can always do it later and if you don't undo it, it'll become really hard to rebase**
If you choose to drop the last commit,
git reset --hard HEAD^
Then, Do
git rebase -i HEAD~13
You will get this screen
pick 9341df2 Someone else's change
pick 1f6b377 Your first commit
... 11 commits
pick 40e4370 Your last commit
Change each of your's commit's pick
to f
(for fixup) and your first commit's pick
to r
for reword
pick 9341df2 Someone else's change
r 1f6b377 Your first commit
... 11 more commits with f
f 40e4370 Your last commit
Save it and you'll get a screen with your first commit message written. Change to to like WIP: Kotlin Migration
and save
Now, your 12 commits will be squashed to 1
Then, do this
git fetch upstream development
git rebase
git fetch upstream development
git rebase upstream/development
And hope nothing breaks. Most probably it will if you did not drop the last commit
Hi @sriramr98!
It looks like one or more of your builds have failed. I've added the relevant info below to save you some time.
failed
Hi @sriramr98!
It looks like one or more of your builds have failed. I've added the relevant info below to save you some time.
failed
@sriramr98 I have updated the PR, first fix the issues mentioned in my review and then move POJOs to new module
Hi @sriramr98!
It looks like one or more of your builds have failed. I've added the relevant info below to save you some time.
failed
This PR is null anyway. If we move the data classes to a new module with lombok we aren't removing lombok. So maybe I should fix things and send the PR to #2357 as we will be adding Kotlin to the main app module.
This PR is null anyway?
Not sure I got you.
If we move the data classes to a new module with lombok we aren't removing lombok.
We don't have the option to. Kotlin is plain and simple not compatible with JSONAPI spec library OR realm
So maybe I should fix things and send the PR
This is the first step to #2357
There is no need to send any new PR or target any other issue. This PR has taken long enough, we should wrap things up as quickly as possible
This is the checklist:
@JvmField
and others)On it. Will finish this PR once and for all by tonight
Hi @sriramr98!
It looks like one or more of your builds have failed. I've added the relevant info below to save you some time.
failed
@sriramr98 I have streamlined everything to bare minimum changes. Do any further changes required and check if app is working as expected
Hi @iamareebjamal!
It looks like one or more of your builds have failed. I've added the relevant info below to save you some time.
failed
Hi @iamareebjamal!
It looks like one or more of your builds have failed. I've added the relevant info below to save you some time.
failed
Hi @iamareebjamal!
It looks like one or more of your builds have failed. I've added the relevant info below to save you some time.
failed
Hi @sriramr98!
It looks like one or more of your builds have failed. I've added the relevant info below to save you some time.
failed
@sriramr98 Please fix any intermediate error and update the PR
Fixes #2371 Changes: Removed all Lombok annotations and replaced them with appropriate getters, setters, constructors, toString and hashCode methods.