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-3180 Remove examples, schema, and expansions from gem package #116

Merged
merged 4 commits into from
Oct 14, 2024

Conversation

360dgries
Copy link
Contributor

Removes the following files from the gem package:

  • The entire examples directory
  • The entire definitions/schema directory
  • Each of the expansions.json of each version

The built gem has gone from 52MB -> 39 MB. Still a lot, but at least it is a reduction. It also built/installed a LOT faster than it has in the past for me.

I have built the gem, installed it, and ran the unit tests in both models and client. Models unit tests uses require instead of require_relative, so I believe it attempts to use the installed gem before the local repo, but I'm not familiar enough with ruby to confirm this. In any case, both pass, but I'm not sure if there is more that should be done to verify this isn't a breaking change.

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.

Add the ticket number to the PR title.

Models unit tests uses require instead of require_relative

If you look in the Gemfile, it says gemspec near the top, which tells bundler to load any local .gemspec files it finds. Bundler finds fhir_models.gemspec and loads it, so it knows that that the current repo is the source for fhir_models. So you can require 'fhir_models/some/arbitrary/path' to require the file lib/fhir_models/some/arbitrary/path.rb.

@360dgries 360dgries changed the title Remove examples, schema, and expansions from gem package FI-3180 Remove examples, schema, and expansions from gem package Oct 3, 2024
@360dgries
Copy link
Contributor Author

I've since made a file with all of the readme code snippets, and compared the outputs. I ran them outside of the fhir_models repo, and compared the outputs. There is no change in behavior except the published version does not have version modules, which is to be expected. Below is the script used that contained all of the readme examples.

require 'fhir_models'

xml = File.read('patient-example.xml')
patient = FHIR.from_contents(xml)
puts patient.to_xml

json = File.read('patient-example.json')
patient = FHIR.from_contents(json)
puts patient.to_json

obs = FHIR::Observation.new(
  'status' => 'final',
  'code' => {
    'coding' => [{ 'system' => 'http://loinc.org', 'code' => '3141-9', 'display' => 'Weight Measured' }],
    'text' => 'Weight Measured'
  },
  'category' => {
    'coding' => [{ 'system' => 'http://hl7.org/fhir/observation-category', 'code' => 'vital-signs' }]
  },
  'subject' => { 'reference' => 'Patient/example' },
  'context' => { 'reference' => 'Encounter/example' }
)
obs.valueQuantity = FHIR::Quantity.new(
  'value' => 185,
  'unit' => 'lbs',
  'code' => '[lb_av]',
  'system' => 'http://unitsofmeasure.org'
)

patient.each_element do |value, metadata, path|
  puts "Info for #{path}:"
  puts "- value: #{value}"
  puts "- type: #{metadata['type']}"
  puts "- cardinality: #{metadata['min']}..#{metadata['max']}"
end

patient.valid? # returns true or false
patient.validate

sd = FHIR::Definitions.resource_definition('Patient')
sd.validates_resource?(patient) # passing in FHIR::Patient
# Validation failed? Get the errors and warnings...
puts sd.errors
puts sd.warnings

@360dgries
Copy link
Contributor Author

Let me know what other testing I should run before this is ready -- Can I force the unit tests to use the installed gem (is that even something we care about/want to do?)

@360dgries 360dgries requested a review from Jammjammjamm October 4, 2024 15:13
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 don't think any of these need to be in the gem:

 ".codeclimate.yml",
 ".gitattributes",
 ".github/workflows/ruby.yml",
 ".gitignore",
 ".rspec",
 ".rubocop.yml",
 ".rubocop_todo.yml",
 ".ruby-version",
 ".simplecov",
 ".tool-versions",
 "Gemfile",
 "Gemfile.lock",
 "Guardfile",
 "README.md",
 "Rakefile",
 "fhir_models.gemspec",

Also, what about these? I don't see them referenced anywhere that makes it look like they're used at runtime.

 "lib/fhir_models/definitions/structures/extension-definitions.json",
 "lib/fhir_models/definitions/structures/profiles-others.json",
 "lib/fhir_models/definitions/structures/profiles-resources.json",
 "lib/fhir_models/definitions/structures/profiles-types.json",
 "lib/fhir_models/definitions/structures/search-parameters.json",

spec.files.reject! { |file| file =~ /lib\/fhir_models\/examples|lib\/fhir_models\/definitions\/schema|lib\/fhir_models\/definitions\/valuesets/}
spec.files.reject! { |file| file =~ /lib\/fhir_models\/igs\/hl7.fhir.r4.core\/package_supplement\/expansions.json/}
spec.files.reject! { |file| file =~ /lib\/fhir_models\/igs\/hl7.fhir.r4b.core\/package_supplement\/expansions.json/}
spec.files.reject! { |file| file =~ /lib\/fhir_models\/igs\/hl7.fhir.r5.core\/package_supplement\/expansions.json/}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we do this so that it isn't hard coded and won't break when we add a new version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the regex to be version agnostic.

I've also changed the initial file gathering to be limited to those in lib, so none of the ones listed above are included anymore.

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 that looks good, but we should still include LICENSE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added back

@360dgries 360dgries requested a review from Jammjammjamm October 9, 2024 19:19
@Jammjammjamm Jammjammjamm force-pushed the FI-3180-Remove-Artifacts-From-Gem branch from a437e97 to a570913 Compare October 11, 2024 13:23
@Jammjammjamm
Copy link
Contributor

@yunwwang @360dgries

I just pushed up a commit which switches to lazily loading the FHIR packages. This gets the start time back in line with what it was before the multi-version update, and then users who don't need the functionality which requires those packages will never have to load them.

CPU Time to load R4 FHIR models:
v4.3.0 - 0.32s (single version)
07db9c09 - 0.98s (current branch w/o lazy loading)
a437e97a - 0.35s (current branch w/lazy loading

@360dgries 360dgries merged commit b4c8d01 into master Oct 14, 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