Add AssertImportHook.is_package() method #136

Closed
wants to merge 10 commits into
from

Conversation

Projects
None yet
4 participants

dahlia commented Jul 10, 2012

Although implementing .is_package() is optional according to PEP 302, it makes several Python codes that assume every import hook implements this method (e.g. Flask) to work well with Attest.

@dahlia dahlia Add AssertImportHook.is_package() method
Implementing .is_package() is optional according to PEP 302,
but it makes several Python codes that assume import hook has
this method (e.g. Flask) to work well with Attest.
f68ed2c
Collaborator

SimonSapin commented Jul 11, 2012

@dahlia , thanks for working on this but nobody really is maintaining Attest at the moment, see #131

Collaborator

bradleyayers commented Jul 12, 2012

I'll have a look over this.

Collaborator

bradleyayers commented Jul 12, 2012

After reading PEP 302 (thanks for the link) it appears that your patch doesn't properly implement the spec.

It says "All three methods should raise ImportError if the module wasn't found.". If you have a look at the existing code for get_source you'll find:

try:
    (fd, fn, info), path = self._cache[name]
except KeyError:
    raise ImportError(name)

I think this pattern should be repeated for is_package.

It would also seem that we should add support for get_code, as per "… each of which should be implemented, or none at all". While were at it it's probably worth adding get_filename too.

dahlia commented Jul 13, 2012

Thanks for your review. AssertImportHook.get_source() method currently returns a triple of (code, filename, newpath) instead of a source string. Should it be changed too?

Collaborator

bradleyayers commented Jul 13, 2012

From reading the PEP, that definitely seems to be the case. I wonder why it was originally written to return (code, filename, newpath), and ideas?

dahlia commented Jul 13, 2012

As I guessed from 5109379, this method was originally written for internal use, not for implementing the loader protocol, and it chose the name get_source by accident.

We can simply rename this method. Any suggestions for its new name?

Collaborator

bradleyayers commented Jul 13, 2012

I think we could probably get away with just implementing the other methods (get_filename, etc) and refactor load_module to use those. Then we can make get_source just return the file contents as it's suppose to.

dahlia commented Jul 13, 2012

Implemented get_filename(), get_code() methods, and refactored load_module(), get_source() methods.

@bradleyayers bradleyayers and 1 other commented on an outdated diff Jul 13, 2012

attest/hook.py
@@ -300,19 +307,50 @@ def get_source(self, name):
except KeyError:
raise ImportError(name)
- code = filename = newpath = None
+ code = None
@bradleyayers

bradleyayers Jul 13, 2012

Collaborator

Instead of storing to code and returning at the end, you could just return from within each if/elif, would save a couple of lines.

@dahlia

dahlia Jul 13, 2012

Actually I firstly wrote to return from within each case, but when I saw the original code it was written in such way (returning at th end), so I followed the way. Okay, I’ll change this anyway. Thank you!

@bradleyayers bradleyayers commented on an outdated diff Jul 13, 2012

attest/hook.py
code = f.read()
+
+ return code
+
+ def is_package(self, name):
+ try:
+ (fd, fn, info), path = self._cache[name]
+ except KeyError:
+ return ImportError(name)
+ return info[2] == imp.PKG_DIRECTORY
+
+ def get_filename(self, name):
+ try:
@bradleyayers

bradleyayers Jul 13, 2012

Collaborator

Same here, just return from within each if/elif

@bradleyayers bradleyayers and 1 other commented on an outdated diff Jul 13, 2012

attest/hook.py
- return code, filename, newpath
+ return filename
+
+ def get_code(self, name):
+ source = self.get_source(name)
+ filename = self.get_filename(name)
+
+ if source:
+ code = compile(source, filename)
+ else:
+ with open(filename, 'rb') as f:
+ f.seek(8)
@bradleyayers

bradleyayers Jul 13, 2012

Collaborator

Excuse my ignorance, but is this actually right? Do you have a link to documentation that says this is appropriate?

@bradleyayers

bradleyayers Jul 13, 2012

Collaborator

Thanks for the links, I agree with what you did, looks good to me!

Collaborator

bradleyayers commented Jul 13, 2012

Overall I really like this, my only criticism now is the lack of tests. I'm not sure the best strategy for testing it, any thoughts?

dahlia commented Jul 13, 2012

Actually I have no idea how to test it correctly and well.

@bradleyayers bradleyayers commented on an outdated diff Jul 13, 2012

attest/hook.py
with fd:
- code = fd.read()
+ return fd.read()
+ elif info[2] == imp.PKG_DIRECTORY:
+ with open(self.get_filename(name), 'U') as f:
+ return f.read()
+
+ def is_package(self, name):
+ try:
+ (fd, fn, info), path = self._cache[name]
+ except KeyError:
+ return ImportError(name)
@bradleyayers

bradleyayers Jul 13, 2012

Collaborator

This should be raise, not return.

@bradleyayers bradleyayers and 1 other commented on an outdated diff Jul 13, 2012

attest/hook.py
+ elif info[2] == imp.PKG_DIRECTORY:
+ with open(self.get_filename(name), 'U') as f:
+ return f.read()
+
+ def is_package(self, name):
+ try:
+ (fd, fn, info), path = self._cache[name]
+ except KeyError:
+ return ImportError(name)
+ return info[2] == imp.PKG_DIRECTORY
+
+ def get_filename(self, name):
+ try:
+ (fd, fn, info), path = self._cache[name]
+ except KeyError:
+ return ImportError(name)
@bradleyayers

bradleyayers Jul 13, 2012

Collaborator

raise not return.

@dahlia

dahlia Jul 13, 2012

Oh, I really was dumb.

Collaborator

bradleyayers commented Jul 13, 2012

The loader.get_code(fullname) method should return the code object associated with the module, or None if it's a built-in or extension module.

How should we handle this? How do we determine if a fullname refers to a built-in or extension module?

Collaborator

bradleyayers commented Jul 16, 2012

So where should we go from here? I haven't had a chance to test this with anything that's likely to expose problems (e.g. with Flask) and I'd like to do that before merging. Have you had any great ideas on how to automated test this?

dahlia commented Jul 16, 2012

I wrote a small test for get_code() and made it to return None when the name is a builtin or an extension. Is this test too naive?

leiserfg commented Feb 2, 2013

I resolve this problem by adding the method is_package to the AssertImportHook in the hook.py file, this is my snippet:

    def is_package(self, name):
        try:
            info = self._cache[name][0][2]
        except KeyError:
            raise ImportError(name)
        return info[2] == imp.PKG_DIRECTORY

It's work fine with flask, please add it to the next Attest version.

shuhaowu referenced this pull request in maxcountryman/flask-login Jun 5, 2013

Closed

Running tests with Flask 0.9 fails #73

dahlia closed this Sep 20, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment