-
Notifications
You must be signed in to change notification settings - Fork 109
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
feat: Refactor CLI to fit Command Pattern #303
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.
Overall LGTM. I have one question regarding the switch to the -bqrh flag but aside from that Approve.
-sc my_bq_conn -tc my_bq_conn \ | ||
-tbls bigquery-public-data.new_york_citibike.citibike_trips,bigquery-public-data.new_york_citibike.citibike_stations \ | ||
--sum tripduration,start_station_name --count tripduration,start_station_name \ | ||
-rc pso-kokoro-resources.pso_data_validator.results | ||
-bqrh pso-kokoro-resources.pso_data_validator.results |
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've seen inconsistencies with the -bqrh flag on the develop branch. Do we have a test that this is currently functional?
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, the --bqrh flag runs exactly the same as the -rc flag using the cli_tools.get_result_handler()
function. We have a test for it in test_cli_tools.py
. The code will first check to see if the --bqrh flag was supplied; if not, it will check if the --rc flag exists; If neither are provided it will set to None.
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.
LGTM
Closes #279
Updates DVT to use validation type specific commands like so:
Updated documentation and examples to reflect the change. It is backwards compatible to continue support for the
data-validation run
command.