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.

Sign up for GitHub

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

Jump to bottom

AC-634 Migrated POJO classes to Kotlin #675

Merged
merged 1 commit into from
Apr 4, 2020
Merged

AC-634 Migrated POJO classes to Kotlin #675

merged 1 commit into from
Apr 4, 2020

Conversation

rishabh-997
Copy link
Collaborator

Description of what I changed

  1. I migrated 38/40 classes to Kotlin to remove unnecessary boilerplate code
  2. Updated other files respectively in order to run tests successfully as Kotlin does not support null assignment
  3. Since null could not be assigned to nonnull Bitmap variable, so I didnt migrated two base POJO files to Kotlin.

Issue I worked on

JIRA Issue: https://issues.openmrs.org/browse/AC-634

Checklist: I completed these to help reviewers :)

  • My pull request only contains ONE single commit
    (the number above, next to the 'Commits' tab is 1).
  • I have added tests to cover my changes. (If you refactored
    existing code that was well tested you do not have to add tests)
  • All new and existing tests passed.
  • My pull request is based on the latest changes of the master branch.

@rishabh-997
Copy link
Collaborator Author

@f4ww4z sir, please review the changes😅😅. Had a tough time migrating it to Kotlin as it is very difficult to handle null point exceptions in Kotlin. I have passed all 104 tests locally.
@everyone, I will squash the commits once all the changes are completed and no more changes are requested...

@codecov-io
Copy link

codecov-io commented Jan 16, 2020 •
edited
Loading

Codecov Report

Merging #675 into master will decrease coverage by 0.36%.
The diff coverage is 35.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #675      +/-   ##
==========================================
- Coverage   12.36%   11.99%   -0.37%     
==========================================
  Files         236      233       -3     
  Lines       10857    10414     -443     
  Branches     1002     1002              
==========================================
- Hits         1342     1249      -93     
+ Misses       9434     9084     -350     
  Partials       81       81
Impacted Files Coverage Δ
...s/mobile/databases/entities/ObservationEntity.java 0% <ø> (ø) ⬆️
...enmrs/mobile/databases/entities/PatientEntity.java 0% <ø> (ø) ⬆️
...nmrs/mobile/databases/entities/LocationEntity.java 0% <ø> (ø) ⬆️
...openmrs/mobile/databases/entities/VisitEntity.java 0% <ø> (ø) ⬆️
...mrs/mobile/databases/entities/EncounterEntity.java 0% <ø> (ø) ⬆️
...enmrs/mobile/databases/entities/ConceptEntity.java 0% <ø> (ø) ⬆️
...ies/visitdashboard/VisitExpandableListAdapter.java 0% <ø> (ø) ⬆️
.../src/main/java/org/openmrs/mobile/models/Answer.kt 0% <0%> (ø)
...nt/src/main/java/org/openmrs/mobile/models/Link.kt 0% <0%> (ø)
...rc/main/java/org/openmrs/mobile/models/FormData.kt 0% <0%> (ø)
... and 86 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cac05ed...31a3fcd. Read the comment docs.

f4ww4z
f4ww4z requested changes Jan 19, 2020
View reviewed changes
Copy link
Collaborator

@f4ww4z f4ww4z left a comment

Choose a reason for hiding this comment

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

@rishabh-997 These are my current reviews... haven't checked everything yet, since there's 81 files being changed. Also, make sure to apply my comments to other files if there's a similar pattern.

openmrs-client/build.gradle Outdated Show resolved Hide resolved
openmrs-client/build.gradle Outdated Show resolved Hide resolved
openmrs-client/src/main/java/org/openmrs/mobile/models/Resource.kt Outdated Show resolved Hide resolved
openmrs-client/src/main/java/org/openmrs/mobile/models/User.kt Outdated Show resolved Hide resolved
openmrs-client/src/main/java/org/openmrs/mobile/models/User.kt Outdated Show resolved Hide resolved
openmrs-client/src/main/java/org/openmrs/mobile/models/Visit.kt Show resolved Hide resolved
openmrs-client/src/main/java/org/openmrs/mobile/models/Visit.kt Outdated Show resolved Hide resolved
openmrs-client/src/main/java/org/openmrs/mobile/models/VisitType.kt Outdated Show resolved Hide resolved
openmrs-client/src/test/java/org/openmrs/mobile/test/ACUnitTestBase.java Outdated Show resolved Hide resolved
openmrs-client/src/main/java/org/openmrs/mobile/models/SystemSetting.kt

class SystemSetting : Resource() {

@Expose
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to serialize every variable in model classes anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think its important because of following line in the code

openmrs-contrib-android-client/openmrs-client/src/main/java/org/openmrs/mobile/api/RestServiceBuilder.java

Line 86 in d786247

.excludeFieldsWithoutExposeAnnotation()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What are your suggestions to this @f4ww4z sir ?

@f4ww4z
Copy link
Collaborator

f4ww4z commented Feb 20, 2020

@rishabh-997 how is your progress for this?

@rishabh-997
Copy link
Collaborator Author

@rishabh-997 how is your progress for this?

Actually I was trying to resolve the above conversations but faced many null pointer exceptions and then got engaged in admission_Form PR. I will continue this from next week after my university exams...

f4ww4z
f4ww4z requested changes Mar 22, 2020
View reviewed changes
Copy link
Collaborator

@f4ww4z f4ww4z left a comment

Choose a reason for hiding this comment

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

@rishabh-997 please address these comments

  1. Remove redundant custom getters and setters. Only use custom getters for computed properties. See this doc.
  2. Remove star imports.

Just tested this and the app runs fine.

openmrs-client/src/main/java/org/openmrs/mobile/models/Encounter.kt Outdated
import com.google.gson.annotations.SerializedName
import org.openmrs.mobile.utilities.DateUtils
import java.io.Serializable
import java.util.*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please avoid star imports.

openmrs-client/src/main/java/org/openmrs/mobile/models/Encounter.kt Outdated

override var uuid: String?
get() = uuid
set(uuid) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

These getters and setters are redundant. Remove them.

@rishabh-997 rishabh-997 requested a review from f4ww4z March 27, 2020 13:42
@rishabh-997
resolving conflicts
31a3fcd 
Updated User.kt

Using lateinit to avoid initialization

removed unused imports

removed redundant getters

squashing commits
@rishabh-997 rishabh-997 changed the title AC 634 Migrated POJO classes to Kotlin AC-634 Migrated POJO classes to Kotlin Mar 31, 2020
f4ww4z
f4ww4z approved these changes Apr 4, 2020
View reviewed changes
Copy link
Collaborator

@f4ww4z f4ww4z left a comment

Choose a reason for hiding this comment

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

LGTM @rishabh-997 !

@f4ww4z f4ww4z merged commit 28bd0b3 into openmrs:master Apr 4, 2020
@rishabh-997 rishabh-997 deleted the AC-634-kotlin branch April 4, 2020 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@f4ww4z f4ww4z f4ww4z approved these changes

@AdamGrzybkowski AdamGrzybkowski Awaiting requested review from AdamGrzybkowski

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rishabh-997 @codecov-io @f4ww4z