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

AC-832 Migrating type converter package to kotlin and fixing minor issue with type converters #811

Merged
merged 3 commits into from
Sep 23, 2020

Conversation

LuGO0
Copy link
Collaborator

@LuGO0 LuGO0 commented Sep 6, 2020

Description of what I changed

  1. Fixed some irregularities in type converters
  2. Deleted one redundant type converter
  3. Converting the package to kotlin

Issue I worked on

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

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.

@LuGO0
Copy link
Collaborator Author

LuGO0 commented Sep 6, 2020

@f4ww4z @rishabh-997 please check up the changes once before I convert them into kotlin!!

@codecov-commenter
Copy link

codecov-commenter commented Sep 6, 2020

Codecov Report

Merging #811 into master will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #811   +/-   ##
=======================================
  Coverage   13.64%   13.64%           
=======================================
  Files         237      236    -1     
  Lines        9552     9552           
  Branches      917      918    +1     
=======================================
  Hits         1303     1303           
  Misses       8143     8143           
  Partials      106      106           
Impacted Files Coverage Δ
.../models/typeConverters/AllergyReactionConverter.kt 0.00% <0.00%> (ø)
...ile/models/typeConverters/FormResourceConverter.kt 0.00% <0.00%> (ø)
.../models/typeConverters/ObservationListConverter.kt 0.00% <0.00%> (ø)
...le/models/typeConverters/PersonAddressConverter.kt 0.00% <0.00%> (ø)
.../models/typeConverters/PersonAttributeConverter.kt 0.00% <0.00%> (ø)
...rs/mobile/models/typeConverters/PersonConverter.kt 0.00% <0.00%> (ø)
...obile/models/typeConverters/PersonNameConverter.kt 0.00% <0.00%> (ø)
...odels/typeConverters/ProviderAttributeConverter.kt 0.00% <0.00%> (ø)
.../org/openmrs/mobile/utilities/PatientComparator.kt 0.00% <0.00%> (ø)
... and 7 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 13233e8...463c715. Read the comment docs.

Copy link
Collaborator

@rishabh-997 rishabh-997 left a comment

Choose a reason for hiding this comment

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

Changes are fine, you can migrate them to kotlin 👍

@LuGO0
Copy link
Collaborator Author

LuGO0 commented Sep 14, 2020

@rishabh-997 i will convert them into kotlin soon just caught up in some work, will make the PR ready for review then 👍

@LuGO0 LuGO0 marked this pull request as ready for review September 16, 2020 19:25
@LuGO0 LuGO0 changed the title [WIP] AC-832 Migrating type converter package to kotlin and fixing minor issue with type converters AC-832 Migrating type converter package to kotlin and fixing minor issue with type converters Sep 16, 2020
Copy link
Collaborator

@rishabh-997 rishabh-997 left a comment

Choose a reason for hiding this comment

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

Nice work @LuGO0, can you try to fix the Travis failure...

@LuGO0
Copy link
Collaborator Author

LuGO0 commented Sep 16, 2020

@rishabh-997 I think its not a travis failure but actually travis failed to start test can you restart the travis test for this commit !!

@LuGO0
Copy link
Collaborator Author

LuGO0 commented Sep 18, 2020

@rishabh-997 I rebased its fine now :)

@LuGO0
Copy link
Collaborator Author

LuGO0 commented Sep 22, 2020

@rishabh I encountered No NPEs while normal usage.

@rishabh-997 rishabh-997 merged commit 4752978 into openmrs:master Sep 23, 2020
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