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

added functionality to merge clinical trial metadata objects #114

Merged
merged 4 commits into from Aug 7, 2019

Conversation

@jim-bo
Copy link
Contributor

commented Aug 6, 2019

No description provided.

@jim-bo jim-bo requested a review from jacoblurye Aug 6, 2019

for d in key_details:
if d in patch and d in target:
if patch[d] != target[d]:
raise RuntimeError("unable to merge trials with different \

This comment has been minimized.

Copy link
@curlup

curlup Aug 6, 2019

What if there will be two trials in different lead organizations but with the same lead_organization_study_id?

This comment has been minimized.

Copy link
@jim-bo

jim-bo Aug 7, 2019

Author Contributor

this ID is globally unique

@jacoblurye
Copy link
Collaborator

left a comment

Looks close! Just a couple comments.

# first we assert the trial details are the same
key_details = ["lead_organization_study_id"]
for d in key_details:
if d in patch and d in target:

This comment has been minimized.

Copy link
@jacoblurye

jacoblurye Aug 6, 2019

Collaborator

If d isn't in patch or isn't in target, then the function continues on happily and tries to merge -- I guess this missing ID would be caught by the validator below, but it's a little hard to follow that in the code. I would either add a comment to that effect, condense this condition into a one-liner with line 899 a la if patch.get(d) != target.get(d), or else raise an error related to missing an expected key.

This comment has been minimized.

Copy link
@jim-bo

jim-bo Aug 7, 2019

Author Contributor

Ok I've added explicit pre-merge validation to each object. This will report any errors in validation before merging to reduce confusion.

ct1 = copy.deepcopy(CLINICAL_TRIAL)
ct2 = copy.deepcopy(CLINICAL_TRIAL)

# first we assert the merge is only happening on the same trial

This comment has been minimized.

Copy link
@jacoblurye

jacoblurye Aug 6, 2019

Collaborator

Should we should also test the non-schema-conformant patch or target case?

This comment has been minimized.

Copy link
@jim-bo

jim-bo Aug 7, 2019

Author Contributor

added this as well. pre-merge validation will stop the merge before it happens.

@jim-bo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

@jacoblurye addressed comments.

@jacoblurye
Copy link
Collaborator

left a comment

Looks good to me!

@jim-bo jim-bo merged commit 22b60a5 into master Aug 7, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jim-bo jim-bo deleted the trial_merge branch Aug 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.