-
Notifications
You must be signed in to change notification settings - Fork 9
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
[GEN-868] add year death validation #560
Conversation
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.
Thanks for doing this! Just some initial comments.
Edit: Could you add more details to the PR description?
Will do |
9e41546
to
41b0356
Compare
if haveColumn: | ||
error = _check_year_death_validity(clinicaldf=clinicaldf) | ||
total_error.write(error) | ||
|
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.
A stretch goal outside of the explicit scope of this change would be to split out these other items into their own functions. Is there a tech debt jira that already exists, or could one be created to address this item?
Cc: @rxu17
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.
Yes we have a tech debt jira/part of the overarching goal for this. The goal is to eventually have a validation function for each validation rule for each fileformat especially when we want to implement GX framework for this.
I think this is part of our documentation epic as well. To break up the validation code so we can document each validation rule individually and completely.
This is not just happening here but throughout the genie code (also for processing as well) but we definitely want to start with validation first.
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.
Awesome work! Just a few formatting and follow up items I noted.
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.
🔥 I'm going to pre-approve! great work here!
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.
Also going to pre-approve! (just have the row-based validation implementation and addressing some of the comments)
Quality Gate passedIssues Measures |
Problem:
Lack of validation check on whether YEAR_DEATH >= YEAR_CONTACT
Solution:
Add the validation check to compare YEAR_DEATH and YEAR_CONTACT when both of them exist and not NAs.
Testing:
Unit test has been added.