Improve error reporting for packaging.util.resolve_name

[Natim]

BPO	12703
Nosy	@tarekziade, @merwok, @embray, @ericsnowcurrently
Files	packaging.util.resolve_name.patch: Patch the packaging.util.resolve_name to display the right exception. test_util.patch resolve-name-errors.diff change-resolve-name.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/merwok'
closed_at = <Date 2014-03-12.09:48:28.812>
created_at = <Date 2011-08-06.10:35:35.325>
labels = ['easy', 'type-bug']
title = 'Improve error reporting for packaging.util.resolve_name'
updated_at = <Date 2014-03-12.09:48:28.811>
user = 'https://bugs.python.org/Natim'

bugs.python.org fields:

activity = <Date 2014-03-12.09:48:28.811>
actor = 'eric.araujo'
assignee = 'eric.araujo'
closed = True
closed_date = <Date 2014-03-12.09:48:28.812>
closer = 'eric.araujo'
components = ['Distutils2']
creation = <Date 2011-08-06.10:35:35.325>
creator = 'Natim'
dependencies = []
files = ['22962', '23428', '23469', '23735']
hgrepos = ['59', '84']
issue_num = 12703
keywords = ['patch', 'easy']
message_count = 18.0
messages = ['141715', '141717', '142533', '142534', '142535', '142601', '142602', '145681', '145696', '145722', '145829', '145955', '147996', '152750', '152865', '164566', '164567', '213233']
nosy_count = 6.0
nosy_names = ['tarek', 'eric.araujo', 'alexis', 'erik.bray', 'eric.snow', 'Natim']
pr_nums = []
priority = 'normal'
resolution = 'out of date'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue12703'
versions = ['3rd party', 'Python 3.3']

[Natim]

I patched it for redbarrel but there is the same bug on distutils2.

This patch allow to see the error raised when loading the module instead of saying that the module does not exists.

https://bitbucket.org/tarek/redbarrel/changeset/2cf6d7e32081#chg-redbarrel/util.py

[merwok]

Thanks for the report. Would you have time to make a code+test patch for the CPython repo? distutils2 has moved there under the name packaging.

[Natim]

Hello, I did the patch, but I have no idea of how to make a test for it.

More over, I have seen a similar problem each time there is this code in the Python code (here in distutils.util.Distribution.get_command_class) :

        try:
            __import__ (module_name)
            module = sys.modules[module_name]
        except ImportError:
            continue

        try:
            klass = getattr(module, klass_name)
        except AttributeError:
            raise DistutilsModuleError(
                  "invalid command '%s' (no class '%s' in module '%s')"
                  % (command, klass_name, module_name))

Maybe it could be better to have a function in cpython to do that ?

[Natim]

Actually it is not the same problem for distutils.util.Distribution.get_command_class my mistake.

[alexis]

Thanks Rémy,

About testing, I would go for modules with errors in it and check that when imported trough this function it does what it is supposed to do.

IOW:

1. Create a test python module with errors in their definition (Throw an exception would do the trick)
   2a. Try to load this python module, check that the exception is the one thrown in the module
   2.b Check that loading a non existing modules still tell us that the module doesn't exist
   2.c Check that importing something existing works, trough this function.

Hope I'm clear enough, thanks again,
Alexis

[merwok]

I’m confused about what the proposed change would actually do. Could you explain what behavior you want to change and how?

[Natim]

It is exactly what explained Alexis.

The __import__ can failed in two case :

• The module doesn't exist
• There is a error in the module

With the previous behaviour, even if the module exist, the message was that it doesn't exists. And it was then not fast forward to guess where was the error.

With this new behaviour, if there is an __import__ error and the module file actually exists, then we raise the real exception problem from the module that we try to import.

[merwok]

I would like to commit this. Tests are needed. Does someone want to write them? Please ask any question you might have, we’re here to help :)

[Natim]

Ok, I am working on it.

[Natim]

Here is the patch for the test case.

[merwok]

Thanks. I’ll add more tests and commit. (I also prefer to create modules in test methods instead of using another file, so that I can see right here what the module contains.)

[merwok]

I have added tests and fixed one or two bugs in 1405df4a1535. I have another patch that checks that error messages are useful, with one exception: if you try to import a.b and b raises an ImportError, then the calling code will see an Attribute error. Right now I don’t see how we can avoid that; checking the existence of files like your patch proposes is not possible, as Python modules do not come from files.

[merwok]

I’ve found a way to make sure error messages always bubble up *and* clean up the ugly code in resolve_name, but it’s a rather drastic one. The idea is to restrict the function to work only with names defined at the module level, not arbitrarily nested names. That way, the code is much easier to write: split on '.', pop the last element and keep it for later, __import__ the rest of the name and look it up in sys.modules. One consequence is that you can’t use package.module.SomeClass.NestedClass for a command, nor module.SomeClass.staticmethod as setup hook; to be pragmatic, I’m not sure that was really useful, and in any case you just have to do a module-level alias (“x = SomeClass.staticmethod”) to make it work.

To reflect the fact that the function has restrictions, I renamed it from the generic “resolve_name” to the vague “find_object”; vague is better because it makes less promises and should cause developers using to look at the docs or docstring.

In short, it’s a clear improvement code-wise that should not impact most of the users, and I like it a lot.

[merwok]

In the absence of feedback, I’m going to apply my find_object idea as described in my previous message.

[merwok]

FYI, here is code that can handle arbitrary dotted names: <http://svn.eby-sarna.com/Importing/peak/util/imports.py?view=markup\>. I haven’t checked if its error reporting has the problem we’re discussing in this report.

An alternative would be to use colon notation, e.g. package.submodule:Thing.Nested.attribute.

My preference is still for find_object, using dots and with the nesting prohibition.

[merwok]

http://bugs.python.org/file25773/resolve_name.patch is a patch that cleans up the function; I’ll see if it can be used to solve our problems without having to follow my drastic find_object idea.

[merwok]

BTW modules in the standard library all use the dotted notation AFAIK, not the colon notation, so I would strongly prefer avoiding the colon notation.

[merwok]

This is now obsolete. For a discussion about moving resolve_name to another part of the stdlib, see bpo-12915.