-
Notifications
You must be signed in to change notification settings - Fork 83
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
Clean up and refactor InvalidTargetDataCheck
#3122
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3122 +/- ##
=======================================
+ Coverage 99.8% 99.8% +0.1%
=======================================
Files 315 315
Lines 30664 30684 +20
=======================================
+ Hits 30573 30593 +20
Misses 91 91
Continue to review full report at Codecov.
|
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 @angela97lin ! Looks good to me!
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.
Looks good to me. Left some minor comments but feel free to ignore.
from .invalid_targets_data_check import InvalidTargetDataCheck | ||
from .invalid_target_data_check import InvalidTargetDataCheck |
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 not sure, but would this count as a breaking change?
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.
Oo good call! I'd say it's safer to add it as a breaking change since we're changing the module name. Adding to release notes, thanks!
Closes #1741
This PR adds a more in-depth docstring detailing what
InvalidTargetDataCheck
checks, and breaks up each of the different checks into their own helper methods.