-
Notifications
You must be signed in to change notification settings - Fork 0
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
Switch _code_system to _system #264
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
cdc.code_system AS category_code_system, | ||
cdc.code_system AS category_system, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach is fine, but I worry that we're setting ourselves up for accidentally using code_system
again down the line by not changing the denorm code now to give .system
instead of .code_system
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
system is a reserved word in sql, which is how we originally got to this place - it'll be fussier, but we can change it there too, though my soft preference is not to
CREATE TABLE core__fhir_mapping_code_system_uri AS SELECT * FROM | ||
CREATE TABLE core__fhir_mapping_system_uri AS SELECT * FROM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stale reference build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah or i saved at the wrong point - will fix.
_code_system
aliases to just_system
#211 with a find/replace of instances of_code_system
in sql and sql templates (excluding the table in fhir_mapping_tables.sql,core__fhir_mapping_code_system_uri
, which is correct in that context.Checklist
docs/
) needs to be updated