classification
Title: Improve error reporting for packaging.util.resolve_name
Type: behavior Stage: resolved
Components: Distutils2 Versions: Python 3.3, 3rd party
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: eric.araujo Nosy List: Natim, alexis, eric.araujo, eric.snow, erik.bray, tarek
Priority: normal Keywords: easy, patch

Created on 2011-08-06 10:35 by Natim, last changed 2014-03-12 09:48 by eric.araujo. This issue is now closed.

Files
File name Uploaded Description Edit
packaging.util.resolve_name.patch Natim, 2011-08-20 14:03 Patch the packaging.util.resolve_name to display the right exception. review
test_util.patch Natim, 2011-10-17 16:09
resolve-name-errors.diff eric.araujo, 2011-10-19 20:25 review
change-resolve-name.diff eric.araujo, 2011-11-20 15:19 review
Repositories containing patches
https://bitbucket.org/natim/cpython
https://bitbucket.org/natim/cpython
Messages (18)
msg141715 - (view) Author: Rémy HUBSCHER (Natim) * Date: 2011-08-06 10:35
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
msg141717 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-08-06 14:41
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.
msg142533 - (view) Author: Rémy HUBSCHER (Natim) * Date: 2011-08-20 14:03
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 ?
msg142534 - (view) Author: Rémy HUBSCHER (Natim) * Date: 2011-08-20 14:23
Actually it is not the same problem for `distutils.util.Distribution.get_command_class` my mistake.
msg142535 - (view) Author: Alexis Metaireau (alexis) * (Python triager) Date: 2011-08-20 15:03
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
msg142601 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-08-21 09:24
I’m confused about what the proposed change would actually do.  Could you explain what behavior you want to change and how?
msg142602 - (view) Author: Rémy HUBSCHER (Natim) * Date: 2011-08-21 09:28
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.
msg145681 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-10-17 12:35
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 :)
msg145696 - (view) Author: Rémy HUBSCHER (Natim) * Date: 2011-10-17 13:54
Ok, I am working on it.
msg145722 - (view) Author: Rémy HUBSCHER (Natim) * Date: 2011-10-17 16:09
Here is the patch for the test case.
msg145829 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-10-18 16:09
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.)
msg145955 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-10-19 20:25
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.
msg147996 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-11-20 15:19
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.
msg152750 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-02-06 17:18
In the absence of feedback, I’m going to apply my find_object idea as described in my previous message.
msg152865 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-02-08 15:02
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.
msg164566 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-07-03 04:36
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.
msg164567 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-07-03 04:37
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.
msg213233 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2014-03-12 09:48
This is now obsolete.  For a discussion about moving resolve_name to another part of the stdlib, see #12915.
History
Date User Action Args
2014-03-12 09:48:28eric.araujosetstatus: open -> closed
resolution: out of date
messages: + msg213233

stage: patch review -> resolved
2012-11-13 04:31:32eric.snowsetnosy: + eric.snow
2012-07-03 04:37:39eric.araujosetmessages: + msg164567
2012-07-03 04:36:29eric.araujosetmessages: + msg164566
2012-05-31 12:03:42tareklinkissue14967 superseder
2012-02-08 15:02:42eric.araujosetnosy: + erik.bray
messages: + msg152865
2012-02-06 17:18:22eric.araujosetmessages: + msg152750
2011-11-20 15:19:34eric.araujosetfiles: + change-resolve-name.diff

messages: + msg147996
stage: test needed -> patch review
2011-10-19 20:25:09eric.araujosetfiles: + resolve-name-errors.diff

messages: + msg145955
2011-10-18 16:44:24eric.araujolinkissue13172 dependencies
2011-10-18 16:09:24eric.araujosetmessages: + msg145829
2011-10-17 16:09:31Natimsetfiles: + test_util.patch
hgrepos: + hgrepo84
messages: + msg145722
2011-10-17 13:54:42Natimsetmessages: + msg145696
2011-10-17 12:35:22eric.araujosetstage: needs patch -> test needed
messages: + msg145681
versions: + 3rd party
2011-08-21 09:28:58Natimsetmessages: + msg142602
2011-08-21 09:24:07eric.araujosetmessages: + msg142601
2011-08-20 15:03:12alexissetmessages: + msg142535
2011-08-20 14:23:51Natimsetmessages: + msg142534
2011-08-20 14:03:39Natimsetfiles: + packaging.util.resolve_name.patch
hgrepos: + hgrepo59
messages: + msg142533

keywords: + patch
2011-08-19 12:45:55eric.araujosetkeywords: + easy
2011-08-06 14:41:05eric.araujosetversions: + Python 3.3
title: Loading a module -> Improve error reporting for packaging.util.resolve_name
messages: + msg141717

assignee: tarek -> eric.araujo
type: behavior
stage: needs patch
2011-08-06 10:35:35Natimcreate