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

FI-2906 R4B and R5 support #169

Merged
merged 12 commits into from
Oct 18, 2024
Merged

FI-2906 R4B and R5 support #169

merged 12 commits into from
Oct 18, 2024

Conversation

360dgries
Copy link
Contributor

Adds R4B and R5 to fhir_client. Also added some unit tests for both versions, autodetection based on fhir_version, etc.

This is dependent on another PR from fhir_models, here.

It is also dependent on the new features introduced in fhir_models, so may need to wait until the next fhir_models gem update.

Copy link
Contributor

@Jammjammjamm Jammjammjamm left a comment

Choose a reason for hiding this comment

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

I think there will be updates needed in lib/fhir_client/ext.

@@ -1,4 +1,6 @@
require 'fhir_models'
require 'fhir_models/r4b'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should only require these when we're going to actually use them, otherwise this undoes the work we did to allow versions to be loaded separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry, Diego, I thought I removed this comment before I finished my review because I decided it was out of scope. Could you undo this part of the changes?

lib/fhir_client/client.rb Outdated Show resolved Hide resolved
@360dgries
Copy link
Contributor Author

Tried my best to remove the multiversion wherever possible. It is still required in some of the unit test files, specifically multiversion_test and set_client_on_resource_test that are actively testing the multiversion capabilities

@@ -12,6 +12,7 @@
Dir.glob(File.join(root, 'fhir_client', 'sections', '**', '*.rb')).each do |file|
require file
end
require_relative 'fhir_client/ext/model' #require first so reference and bundle can inherit
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we remove multiversion loading from the start, there is an issue with superclass matching for the versioned classes of bundle and reference defined in ext. This fixed it, I think it's because of the versioned model definitions occurring late in model.rb for fhir_client, and the versioned classes not being loaded from fhir_models.

If we load multiversion from the start, this is not an issue as we load the model.rb files in models that contain the versioned classes. I've removed the line with the other reverted changes.

@@ -1,4 +1,6 @@
require 'fhir_models'
require 'fhir_models/r4b'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry, Diego, I thought I removed this comment before I finished my review because I decided it was out of scope. Could you undo this part of the changes?

include FHIR::ReferenceExtras

def resource_class
"FHIR::R4B::#{resource_type}".constantize unless contained?
Copy link
Contributor

Choose a reason for hiding this comment

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

FHIR::R4B.const_get(resource_type).

@@ -7,6 +7,10 @@ def versioned_resource_class(klass = nil)
FHIR::STU3
when :dstu2
FHIR::DSTU2
when :r4b
defined?(FHIR::R4B) ? FHIR::R4B : FHIR
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes should be reverted, right?

@360dgries 360dgries merged commit 8fec066 into master Oct 18, 2024
3 checks passed
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