Skip to content
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

Merged
merged 5 commits into from
Oct 10, 2024
Merged

fix CLI and logging #60

merged 5 commits into from
Oct 10, 2024

Conversation

mishaschwartz
Copy link
Contributor

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:

  • Fix bug where logger setup failed
  • Simplify CLI argument constructor code (for cleaner and more testable code)
  • Add tests for CLI and implementations when invoked through the CLI
  • Refactored code dealing with requests and authentication to the requests.py file
  • Add --log_file command line option to specify a non-default location to write log files to

Copy link
Collaborator

@huard huard left a 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.

STACpopulator/cli.py Outdated Show resolved Hide resolved
STACpopulator/implementations/DirectoryLoader/__init__.py Outdated Show resolved Hide resolved
STACpopulator/logging.py Show resolved Hide resolved
STACpopulator/requests.py Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
@mishaschwartz mishaschwartz merged commit ea011a9 into master Oct 10, 2024
9 checks passed
@mishaschwartz mishaschwartz deleted the fix-cli branch October 10, 2024 19:23
)
for implementation_module_name, module in implementation_modules().items():
implementation_parser = populators_subparser.add_parser(implementation_module_name)
module.add_parser_args(implementation_parser)
Copy link
Collaborator

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.

Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants