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

Fix split logic of canonical into url and version in several places #1619

Closed

Conversation

Boereck
Copy link
Contributor

@Boereck Boereck commented May 10, 2024

Removed

  • org.hl7.fhir.r4b.renderers.DataRenderer#versionFromCanonical
  • org.hl7.fhir.r4b.renderers.DataRenderer#systemFromCanonical
  • org.hl7.fhir.r5.renderers.DataRenderer#versionFromCanonical
  • org.hl7.fhir.r5.renderers.DataRenderer#systemFromCanonical

The logic of these methods is wrong, the functionality of extracting the version and the system URL have been swapped.

Therefore, all usages of aforementioned methods were replaced by the use of org.hl7.fhir.utilities.CanonicalPair in org.hl7.fhir.r4b.renderers.TerminologyRenderer and org.hl7.fhir.r5.renderers.TerminologyRenderer. This not only fixes the problem, but also reduces duplicated functionality.

The test case for this change was proposed to the test repository in PR FHIR/fhir-test-cases#172 .
This PR closes the second round of errors found in issue #1611 . After this PR, all found bugs are taken care of.

Removed
- org.hl7.fhir.r4b.renderers.DataRenderer#versionFromCanonical
- org.hl7.fhir.r4b.renderers.DataRenderer#systemFromCanonical
- org.hl7.fhir.r5.renderers.DataRenderer#versionFromCanonical
- org.hl7.fhir.r5.renderers.DataRenderer#systemFromCanonical

The logic of these methods is wrong, the functionality of extracting
the version and the system URL have been swapped.

Therefore, all usages of aforementioned methods were replaced by the
use of org.hl7.fhir.utilities.CanonicalPair. This not only fixes the
problem, but also reduces duplicated functionality.
Copy link

codecov bot commented May 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 12.18%. Comparing base (e57ef59) to head (13a3953).
Report is 20 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #1619   +/-   ##
=========================================
  Coverage     12.18%   12.18%           
- Complexity    31581    31588    +7     
=========================================
  Files          2182     2182           
  Lines        671491   671479   -12     
  Branches     198194   198191    -3     
=========================================
+ Hits          81846    81850    +4     
+ Misses       560217   560206   -11     
+ Partials      29428    29423    -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dotasek
Copy link
Collaborator

dotasek commented Jun 19, 2024

@Boereck this looks good to me. To be merged, it still requires a merge from master.

Since the merge required a little extra work (it would re-add deleted methods) I did it myself here: Boereck#1

Or you could do it yourself to be certain.

@Boereck
Copy link
Contributor Author

Boereck commented Jun 20, 2024

I see that #1663 was merged and contains the changes on the current master. @dotasek : Thanks for taking the time to catch up to the master and merging it in.
I will close this PR, since the issue is taken care of in the other PR.

@Boereck Boereck closed this Jun 20, 2024
@dotasek
Copy link
Collaborator

dotasek commented Jun 20, 2024

@Boereck thanks for closing! I was looking for this today after the master branch built and wondered where it disappeared to.

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