Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

issue-77: add Bundles extractEntry Java api with contained resources #72

Merged
merged 2 commits into from
Feb 10, 2020

Conversation

amarvakul
Copy link
Contributor

Added new Java api for Bundles extractEntry api to accept contained resources. Closes #71

Copy link
Contributor

@bdrillard bdrillard left a comment

Choose a reason for hiding this comment

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

Thanks for these additions. A few minor comments.


FhirContext context = FhirContexts.contextFor(fhirVersion);

this.converter = SparkRowConverter.forResource(context, resourceTypeUrl);
if (this.containedResourceTypeUrls == null || this.containedResourceTypeUrls.size() == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we use this.containedResourceTypeUrls.isEmpty() instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed f0d5fdc

public Dataset<Row> extractEntry(SparkSession spark,
JavaRDD<BundleContainer> bundles,
Class resourceClass,
Class... containedClasses) {
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'll need to make this parameter an explicit list. In practice, Java will have difficulty dispatching between this new method and extractEntry that has the single Class parameter since it can't tell if the call to extractEntry without a final argument is a call without contained classes, or just with empty contained classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed f0d5fdc

Copy link
Contributor

@bdrillard bdrillard left a comment

Choose a reason for hiding this comment

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

+1, thanks!

@amarvakul amarvakul merged commit 78806aa into cerner:0.5.0-dev Feb 10, 2020
@amarvakul
Copy link
Contributor Author

Merged to 0.5.0-dev branch. Thanks for review !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants