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

Iterate over resources #89

Merged
merged 6 commits into from
Jun 15, 2021
Merged

Iterate over resources #89

merged 6 commits into from
Jun 15, 2021

Conversation

Jammjammjamm
Copy link
Contributor

This branch adds an #each_element method to FHIR::Model so that resources can be easily iterated over.

I also updated some dependencies to address security vulnerabilities.

@Jammjammjamm Jammjammjamm self-assigned this Jun 14, 2021
README.md Outdated Show resolved Hide resolved
@arscan
Copy link
Member

arscan commented Jun 14, 2021

Not sure if this is a real issue or not, but pry is now giving me this error when I start up the console (I can successfull ignore it now):

fhir_models % bundle exec rake fhir:console
bin/console
require 'pry-coolline' # Failed, saying: undefined method `file' for #<Pry::History:0x00007fd76fcfa398>
Did you mean?  filter

There is a chance this was already in here from a previous update to our Gemfile.lock and I just didn't notice it. Just FYI

Co-authored-by: Rob Scanlon <robscanlon@gmail.com>
@Jammjammjamm
Copy link
Contributor Author

I saw that too, but I didn't know what to do about it.

Copy link
Member

@arscan arscan left a comment

Choose a reason for hiding this comment

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

Looks good. I also manually verified against a Goal resource from our test data set.

I thought the return value was a little confusing and perhaps it shouldn't be exposed to users (ideally I guess it would return an enumerator or something? or at least that's what each does). No big deal though. Also, I assume that primitive extensions would be ignored here, which might be worth noting in this section of the readme, though we do mention it earlier at a high level too, so I'm ok leaving that out.

I suppose we should backport this to fhir-crucible/fhir_dstu2_models and fhir-crucible/fhir_stu3_models. Hopefully just a copy/paste, but there is a chance that something changed implementaiton-wise that would make this not work? I'm not sure.

Copy link
Contributor

@radamson radamson left a comment

Choose a reason for hiding this comment

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

Tested it out and everything looks good.

Couple things to discuss regarding the method name and return.

lib/fhir_models/bootstrap/model.rb Show resolved Hide resolved
lib/fhir_models/bootstrap/model.rb Show resolved Hide resolved
Co-authored-by: Reece Adamson <41651655+radamson@users.noreply.github.com>
Copy link
Contributor

@radamson radamson left a comment

Choose a reason for hiding this comment

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

Looks good 👍

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.

None yet

3 participants