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.

Sign up for GitHub

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

Jump to bottom

A couple miscellaneous fixes #200

Merged
merged 1 commit into from
Mar 28, 2024
Merged

A couple miscellaneous fixes #200

merged 1 commit into from
Mar 28, 2024

Conversation

mikix
Copy link
Contributor

@mikix mikix commented Mar 27, 2024 •
edited
Loading

I found these while working on tests for a different upcoming PR.

Checklist

  • Consider if documentation (like in docs/) needs to be updated
  • Consider if tests should be added
  • Update template repo if there are changes to study configuration

@mikix mikix marked this pull request as ready for review March 27, 2024 20:58
dogversioning
dogversioning approved these changes Mar 28, 2024
View reviewed changes
Copy link
Contributor

@dogversioning dogversioning left a comment

Choose a reason for hiding this comment

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

Can we modify/add some data to the duckdb json that will exercise these changes?

cumulus_library/studies/core/core_templates/patient.sql.jinja Outdated Show resolved Hide resolved
cumulus_library/studies/core/core_templates/patient.sql.jinja Show resolved Hide resolved
cumulus_library/studies/core/core_templates/documentreference.sql.jinja Outdated Show resolved Hide resolved
cumulus_library/studies/core/core_templates/documentreference.sql.jinja Outdated Show resolved Hide resolved
@mikix
Copy link
Contributor Author

mikix commented Mar 28, 2024

Can we modify/add some data to the duckdb json that will exercise these changes?

Yes, let me add my dumb little test that triggered these. Though it is a bit of a testing departure from the rest of the code - but I think it will be useful. Let me push up and you can comment.

@mikix mikix force-pushed the mikix/low-schema-fixes branch from ff5d584 to 998b3c0 Compare March 28, 2024 14:55
@mikix
Copy link
Contributor Author

mikix commented Mar 28, 2024

OK updated to resolve your comments, and added a little test helper class I was working on, for more dynamic ndjson rather than ndjson that lives on disk in a folder. Felt easier to create little edge cases for. But it's a bit of a departure from the norm in this repo - I can adjust it to your taste.

@mikix mikix force-pushed the mikix/low-schema-fixes branch from 998b3c0 to 7f023c0 Compare March 28, 2024 14:58
dogversioning
dogversioning reviewed Mar 28, 2024
View reviewed changes
Copy link
Contributor

@dogversioning dogversioning left a comment

Choose a reason for hiding this comment

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

I do like that approach for edge cases, it feels less risky than mucking with the source data in terms of spiraling dependencies.

tests/testbed_utils.py
Comment on lines +85 to +87
# TODO: fix the core SQL to check for extensions in the schema
# before querying them. In the meantime, we can just ensure
# those fields exist, ready to be queried.
Copy link
Contributor

Choose a reason for hiding this comment

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

this is on my list but is waiting for the next extension we support.

tests/testbed_utils.py


class LocalTestbed:
def __init__(self, path: Path, with_patient: bool = True):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this path arg default to the tmp_path fixture?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure how, tbh

cumulus_library/studies/core/core_templates/patient.sql.jinja Outdated Show resolved Hide resolved
@mikix
core: handle odd address and DocRef/encounter edge cases better
4094e1c 
We now only use one address per patient, to avoid duplicate
patient entries (and also now allow patients with null addresses).

And we now build DocRefs even if context.encounter isn't in schema,
and allow DocRefs with null encounters.
@mikix mikix force-pushed the mikix/low-schema-fixes branch from 7f023c0 to 4094e1c Compare March 28, 2024 17:13
@mikix mikix merged commit 111da13 into main Mar 28, 2024
3 checks passed
@mikix mikix deleted the mikix/low-schema-fixes branch March 28, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dogversioning dogversioning dogversioning approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mikix @dogversioning