code cleanup

[mishaschwartz]

• fix incorrect example in README
• move argument parsing for logging options to the implementation code
• fix bug where logging options were being set incorrectly
• rename files to avoid potential naming conflicts with other packages (logging and requests)

Resolves #62

[fmigneault]

I think this is potentially dangerous if we add other sub-ArgumentParser commands than the top-level CLI run command which might not have the populator attribute.

Since the run function (not to confuse with the run subparser command) only does implementation_modules()[ns.populator].runner(ns), there's basically no difference from doing runner(ns) (as before), and there's no need to inject the populator argument anymore.

[mishaschwartz]

This is necessary so that the logging options are applied when this file is executed directly from the command line.

I think this is potentially dangerous if we add other sub-ArgumentParser commands than the top-level CLI run command which might not have the populator attribute.

Agreed, if that happens, then we'd have to modify this code as well. The simpler and more maintainable thing to do would be to remove the option to call populator scripts directly from the command line.

[fmigneault]

I'm not sure to understand why the logging options cause this? populator is only defined by the run subparser. What causes populator to become an issue if calling the script directly since run would be bypassed entirely?

[fmigneault]

I gave it a try, and no error on my end (beside the unrelated schema validation of course).

[mishaschwartz]

I'm not sure to understand why the logging options cause this?

There's no error but the logging options are applied here

stac-populator/STACpopulator/cli.py

Line 49 in b353feb

setup_logging(ns.log_file, ns.debug or logging.INFO)

So if you don't run it through the CLI functions then the logging options are never applied. We could have added a call to setup_logging in every runner function as well but that seemed like we're adding boilerplate (which we're actively trying to remove in #64)

[fmigneault]

OK. But can we at least resolve the populator name automatically rather than a hard coded value? I really dislike having different name resolution strategies by different part of the code. If the directory name is the "reference" for the populator name, then everywhere should resolve it the same way.