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-608: rename data source to repositories #589

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
3 participants
@Gelassen
Copy link
Contributor

commented May 28, 2019

Description of what I changed

Just refactoring. See the title.

Blocker for AC-400
#585

Issue I worked on

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

Checklist: I completed these to help reviewers :)

  • My pull request only contains ONE single commit

  • 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.

@Gelassen Gelassen referenced this pull request May 28, 2019

Closed

AC-400: add concepts data providers, add visit form #585

2 of 4 tasks complete
@f4ww4z
Copy link
Collaborator

left a comment

@Gelassen Thanks for the PR, it looks good. Just make sure to remove the changes not linked to the issue.

Show resolved Hide resolved build.gradle Outdated
Show resolved Hide resolved gradle/wrapper/gradle-wrapper.properties Outdated
@f4ww4z

This comment has been minimized.

Copy link
Collaborator

commented May 29, 2019

@Gelassen Please fix the merge conflict and I will merge. Since gradle is updated, just remove any changes in build.gradle.

@codecov-io

This comment has been minimized.

Copy link

commented May 30, 2019

Codecov Report

Merging #589 into master will not change coverage.
The diff coverage is 51.06%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #589   +/-   ##
=======================================
  Coverage   14.38%   14.38%           
=======================================
  Files         184      184           
  Lines        8565     8565           
  Branches      746      746           
=======================================
  Hits         1232     1232           
  Misses       7256     7256           
  Partials       77       77
Impacted Files Coverage Δ
...nmrs/mobile/api/repository/LocationRepository.java 0% <0%> (ø)
...ditpatient/SimilarPatientsRecyclerViewAdapter.java 0% <0%> (ø) ⬆️
...in/java/org/openmrs/mobile/api/PatientService.java 0% <0%> (ø) ⬆️
.../java/org/openmrs/mobile/api/EncounterService.java 0% <0%> (ø) ⬆️
...patients/LastViewedPatientRecyclerViewAdapter.java 0% <0%> (ø) ⬆️
...nmrs/mobile/api/repository/RetrofitRepository.java 100% <100%> (ø)
...enmrs/mobile/api/repository/PatientRepository.java 13.11% <40%> (ø)
...vities/addeditpatient/AddEditPatientPresenter.java 46.49% <50%> (ø) ⬆️
...openmrs/mobile/api/repository/VisitRepository.java 71.42% <50%> (ø)
...penmrs/mobile/activities/login/LoginPresenter.java 65.49% <66.66%> (ø) ⬆️
... and 4 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 3a94d55...7d0e454. Read the comment docs.

@f4ww4z

f4ww4z approved these changes May 30, 2019

Copy link
Collaborator

left a comment

Looks good @Gelassen , I will merge this after 2.7.2 is released. Our priority is to publish to the Play store, do take a look at AC-613 if you have time.

@f4ww4z f4ww4z changed the title AC-608: rename data source to repositories and upate gradle AC-608: rename data source to repositories Jun 4, 2019

@f4ww4z

This comment has been minimized.

Copy link
Collaborator

commented Jun 4, 2019

#586 Is blocking this PR from being merged.

@f4ww4z

This comment has been minimized.

Copy link
Collaborator

commented Jun 6, 2019

@Gelassen Please resolve the merge conflict, then we can merge. Sorry for the delay.

@f4ww4z f4ww4z force-pushed the openmrs:master branch from 5222962 to 61319ec Jun 10, 2019

@f4ww4z

This comment has been minimized.

Copy link
Collaborator

commented Jun 11, 2019

@Gelassen Any updates?

@Gelassen Gelassen force-pushed the Gelassen:feature/AC-608 branch from 102ce37 to 7bcbfef Jun 15, 2019

@f4ww4z

This comment has been minimized.

Copy link
Collaborator

commented Jun 17, 2019

@Gelassen You might need to just reset everything to the latest master branch and refactor it again.

Gelassen and others added some commits Jun 19, 2019

Merge pull request #1 from openmrs/master
Sync up with origin

@Gelassen Gelassen force-pushed the Gelassen:feature/AC-608 branch from 7bcbfef to 7d0e454 Jun 19, 2019

@Gelassen

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

Excuse me for the delay, now it is ready for merge.

@f4ww4z

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2019

@Gelassen There's still a merge conflict here. Make sure to pull the latest changes from master, then redo your changes (i think it's the best solution since I merged other PRs that might use the APIs). See this solution.

@f4ww4z

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

@Gelassen Ping me when you resume working on this.

@f4ww4z f4ww4z closed this Jul 8, 2019

@Gelassen

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

@f4ww4z , the common scenario is pull all changes from master to the forked repository; after that do rebase on master for the current branch and create the PR. Please point out issue or issues in case you see any in this approach

@f4ww4z

This comment has been minimized.

Copy link
Collaborator

commented Jul 9, 2019

@Gelassen , my recommended solution is to just

  1. Hard reset this branch
  2. Make your changes (should be easy, just refactor using Android Studio)
  3. commit
  4. Reopen this PR and claim the issue on JIRA page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.