Unable to make a relative Content Handler import from the command-line

[scottwallacesh]

On the command line, importing a custom Response Handler using a relative path requires manipulation of the PYTHONPATH environment variable to add . to the list of paths.

Should Gabbi allow relative imports to work out-of-the-box?

e.g.

gabbi-run -r foo.bar:ExampleHandler < example.yaml

... fails with, ModuleNotFoundError: No module named 'foo'.

Updating PYTHONPATH...

PYTHONPATH=${PYTHONPATH}:. gabbi-run -r foo.bar:ExampleHandler < example.yaml

... works.

[scottwallacesh]

A very simple workaround would be to include . when initialising the handlers:

e.g.

--- a/gabbi/runner.py
+++ b/gabbi/runner.py
@@ -159,6 +159,7 @@ def run_suite(handle, handler_objects, host, port, prefix, force_ssl=False,

 def initialize_handlers(response_handlers):
+    sys.path.append('.')
     custom_response_handlers = []
     handler_objects = []
     for import_path in response_handlers or []:

[cdent]

/cc @FND

I had thought we already had a work around for this. Will try to refresh my memory.

[cdent]

Apparently we never did address this.

Adding the sys.path append by default would change existing behavior in potentially drastic ways (if someone had the wrong version of an also installed package that just happened to be in .. Would it make any sense to have something like a-l arg that if set says "look locally for response handler (by adding . to sys.path)"? It's effectively shorthand for cooking pythonpath

[scottwallacesh]

We could "detect" a relative import if it was declared as, .foo.bar:... and only update PYTHONPATH for the handler initialisation. Something like...

def initialize_handlers(response_handlers):
    # Save PyPath
    pypath = sys.path

    custom_response_handlers = []
    handler_objects = []
    for import_path in response_handlers or []:
        # Check if a relative import was provided
        if import_path.startswith('.'):
            if '.' not in sys.path:
                # Allow relative imports
                sys.path.append('.')
            # Trim the leading '.' from the import path
            import_path = import_path[1:]
        for handler in load_response_handlers(import_path):
            custom_response_handlers.append(handler)
    for handler in handlers.RESPONSE_HANDLERS + custom_response_handlers:
        handler_objects.append(handler())

    # Restore PyPath
    sys.path = pypath
    return handler_objects

[cdent]

We could "detect" a relative import if it was declared as, .foo.bar:...

That could work but I worry it might be a bit too magical. I suppose it does look enough like a relative import to imply as much.

Any corner cases we're not thinking of?

It's clear enough to document: "If you want to load a module from the local dir prepend a dot..."

[scottwallacesh]

Any corner cases we're not thinking of?

Not that I can think of right now.

[scottwallacesh]

Either implementation would work, IMHO. As you say, a -l argument would be less "magical". We're already having to write -r ... anyway and it would be simple enough to make it, -lr ....

[cdent]

How's this for a bribe? If you implement it and provide a pull request, you get to choose whichever of the two seems better to you and I'll be happy to merge it. Otherwise it might be a few days before I get to it (and I'll end up being the decider :) )

[scottwallacesh]

Oh dear. Many failed tests. I'll have to understand how they're run and work out what's going on there.

[cdent]

Many failed tests.

I think it was just pep8 that failed and since it runs faster than all the others, it cancelled the rest of them so as not to "waste" the test run.

You can do tox -epep8 locally to run the pep8 tests, or just tox to run all the default tests locally.

[scottwallacesh]

I don't have some of the older Python versions available, so some tests failed locally. Everything else looks okay...?

___________________________________________________________________________ summary ____________________________________________________________________________
  pep8: commands succeeded
ERROR:  py36: InterpreterNotFound: python3.6
ERROR:  py37: InterpreterNotFound: python3.7
  py38: commands succeeded
  py39: commands succeeded
ERROR:  pypy3: InterpreterNotFound: pypy3
  limit: commands succeeded
  failskip: commands succeeded
  docs: commands succeeded
  py39-prefix: commands succeeded
  py39-limit: commands succeeded
  py39-verbosity: commands succeeded
  py39-failskip: commands succeeded
ERROR:  py36-pytest: InterpreterNotFound: python3.6
ERROR:  py37-pytest: InterpreterNotFound: python3.7
  py38-pytest: commands succeeded
  py39-pytest: commands succeeded

[cdent]

Yeah, that looks reasonable. I assume this is after you fixed the line too long?

[scottwallacesh]

It wasn't but the Github tests are green now.

[cdent]

I guess this means you've started using content or response handlers? Anything worth sharing?

[scottwallacesh]

Probably not. We were going raise a different issue today with some proposals to allow "raw strings" for response_headers as we wanted to match exact URLs for location:. e.g. /foo/bar/ but as they're "wrapped" in /s they're treated as regex strings which doesn't work for us. The current workaround, /^\/foo\/bar\/$/, was a bit ugly.

We had a proposal to disable the regex functionality but I wanted to check the issue hadn't been raised in the past and that's when I read up about custom handlers. I simply copied the existing HeadersResponseHandler and removed the regex code and updated the test_key_suffix value. That's when I came across the PYTHONPATH issue.

[scottwallacesh]

Of course, if we write an interesting custom handler, we'll share it!

[cdent]

Ah okay. If you had showed up and said "we want a raw strings for response headers thing" I probably would have responded with "make a response handler, and then you can make it do all sort of things you might like". So good to hear you found your own way there.