-
Notifications
You must be signed in to change notification settings - Fork 2
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
fix CLI and logging #60
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.
Gave a quick look, looks good from my end and takes one step toward generalization.
) | ||
for implementation_module_name, module in implementation_modules().items(): | ||
implementation_parser = populators_subparser.add_parser(implementation_module_name) | ||
module.add_parser_args(implementation_parser) |
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.
Starting to look into this, and I'm not really fond of the logic depending on a function named "add_parser_args" being present in the module. I don't have a better alternative yet though.
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.
The previous approach assumed a make_parser
instead. The logic just changed the name to represent what the function does, but has similar dependencies.
The only workaround I can think of is to define an abstract ArgParser
class with a add_parser_args
method, and each implementation must define their ArgParser
inheriting from it. The CLI could then look up the modules and filter by issubclass
of ArgParser
to find relevant references. That being said, it just defines an explicit structure, but the result would be the same.
While experimenting with the CLI I realized that the implementations could not be invoked through the CLI due to an import error in the
logging.py
script.This PR fixes that error and adds tests for the CLI code itself to ensure that these errors are detected earlier. Testing required refactoring the CLI code in order to test it properly. While refactoring the code, I also cleaned it up a bit and enforced some restrictions on return types from functions.
Finally, there was no mechanism to specify an alternative place to write log files, so I added one.
Summary of changes:
requests.py
file--log_file
command line option to specify a non-default location to write log files to