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

US core required/must support review #191

Merged
merged 3 commits into from
Mar 7, 2024
Merged

Conversation

dogversioning
Copy link
Contributor

This PR makes the following changes:

  • Adds remaining fields in US core suppported profile to core tables
  • Moves encounter builder CodeableConcepts to dataclass based instead of dict based
  • --builder cli arg now looks for a substring
  • added --builder to sql generator subparser

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

Sorry, something went wrong.

Comment on lines +15 to +17
types: []
types_or: [sql,jinja]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is very silly - please reinstall the pre-commit hook after this goes in

Copy link
Contributor

Choose a reason for hiding this comment

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

What does this change do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the default pre-commit hook configuration for sqlfluff only scans changes to sql files, and the default pre-commit behavior is to use the 'types' field as an and. So this zeroes out the default behavior, and then sets it to look for either files ending in .sql or in .jinja.

Comment on lines +15 to +17
types: []
types_or: [sql,jinja]
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this change do?

cumulus_library/studies/core/builder_documentreference.py Outdated Show resolved Hide resolved
source_table="documentreference",
source_id="id",
column_name="content.format",
is_array=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

content is an array though - does the generator automatically assume parents are arrays?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch - will update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for posterity - I think this actually requires a larger chunk of effort, since this is a coding rather than a codeable concept and I think they's kind of smashed together at the moment in a way that suggests reusability but doesn't actually provide it.

Since this PR is already huge, I want to take this as is, where it will always just generate a null table, and work on the other thing seperately. #192 to track.

cumulus_library/study_parser.py Outdated Show resolved Hide resolved
Comment on lines +527 to +528
if builder and file.find(builder) == -1:
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: both places you ask for all generators, you immediately filter by builder - should that maybe instead be an argument to the get-generators method?

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'm on the fence - i left it this way in case, in the future, there's some other kind of generator - perhaps something for loading data from flat files, for example.

Comment on lines +263 to +264
|enc_class_code |varchar(6) | |
|enc_class_display |varchar(21)| |
Copy link
Contributor

Choose a reason for hiding this comment

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

What causes these columns to be size-limited? We probably don't want them to be, yeah?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the default behavior of the pytablewriter markdown output generator. I wasn't opinionated enough to dig into the options too much - since most of the time we're just going to regenerate from the DB anyway, i kinda don't care. but this is a very weakly held opinion.

@dogversioning dogversioning merged commit b1346cc into main Mar 7, 2024
3 checks passed
@dogversioning dogversioning deleted the mg/us_core_complaince branch March 7, 2024 19:14
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

2 participants