in pickleshare.py line52 should be "if not os.path.isdir(self.root):"? #737

Closed
fungid opened this Issue Aug 27, 2011 · 18 comments

Projects

None yet

4 participants

@fungid

because of
from python3.2.1 doc: os.path.isdir(path)
Return True if path is an existing directory. This follows symbolic links, so both islink() and isdir() can be true for the same path.

@minrk
IPython member

I'm not sure I understand the question. self.root.path.isdir() is equivalent to os.path.isdir(self.root), since self.root is a path object, that is a convenient wrapper for path strings.

@fperez
IPython member

The semantics of os.path.isdir and the .isdir method of path objects are identical regarding symlinks, as demonstrated here:

In [1]: from IPython.external.path import path as Path

In [2]: Path('adir').isdir()
Out[2]: True

In [3]: Path('alink').isdir()
Out[3]: True

In [4]: import os

In [5]: os.path.isdir('adir')
Out[5]: True

In [6]: os.path.islink('alink')
Out[6]: True

In [7]: d
total 12
drwxr-xr-x 2 fperez 4096 2011-09-12 19:46 adir/
lrwxrwxrwx 1 fperez    4 2011-09-12 19:46 alink -> adir/

Closing as this is not a bug.

@fperez fperez closed this Sep 13, 2011
@fungid

I don't know why,but i use winxp sp3 and python 3.2.2, here is what i got:

WARNING: Readline services not available or not loaded.
WARNING: Proper color support under MS Windows requires the pyreadline library.
You can find it at:
http://ipython.scipy.org/moin/PyReadline/Intro
Gary's readline needs the ctypes module, from:
http://starship.python.net/crew/theller/ctypes
(Note that ctypes is already part of Python versions 2.5 and newer).

Defaulting color scheme to 'NoColor'
Python 3.2.2 (default, Sep  4 2011, 09:51:08) [MSC v.1500 32 bit (Intel)]
Type "copyright", "credits" or "license" for more information.

IPython 0.11 -- An enhanced Interactive Python.
?         -> Introduction and overview of IPython's features.
%quickref -> Quick reference.
help      -> Python's own help system.
object?   -> Details about 'object', use 'object??' for extra details.

In [1]: from IPython.external.path import path as Path

In [2]: Path('__pycache__').isdir()
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
C:\Python32\Scripts\<ipython-input-2-c97e0f958d2b> in <module>()
----> 1 Path('__pycache__').isdir()

TypeError: _isdir() takes exactly 1 argument (0 given)

In [3]: Path('__pycache__').isdir('__pycache__')
Out[3]: True

In [4]:
@fperez
IPython member

Mmh, that's seriously weird. @takluyver, could you test something like

from IPython.external.path import path as Path
Path('/home').isdir()

in your py3 install of ipython. Because I certainly don't get the above problem in py2, and I don't even see how it could happen in the first place (it's as if isdir wasn't being treated like an instance method). So I'm wondering if this is a py3-specific issue...

@fungid

Python 3.2.2 (default, Sep 4 2011, 09:51:08) [MSC v.1500 32 bit (Intel)] on win
32
Type "help", "copyright", "credits" or "license" for more information.

from IPython.external.path import path as Path
Path('c:\windows').isdir()
Traceback (most recent call last):
File "", line 1, in
TypeError: _isdir() takes exactly 1 argument (0 given)
Path('c:\windows').isdir('c:\windows')
True

@takluyver
IPython member
In [3]: Path('/home').isdir()
Out[3]: True

Python 3.2, Ubuntu Natty.

@takluyver
IPython member

@fungid, what do you get if you do this:

In [11]: Path("/home").isdir
Out[11]: <bound method path.isdir of path('/home')>
@fungid
WARNING: Readline services not available or not loaded.
WARNING: Proper color support under MS Windows requires the pyreadline library.
You can find it at:
http://ipython.scipy.org/moin/PyReadline/Intro
Gary's readline needs the ctypes module, from:
http://starship.python.net/crew/theller/ctypes
(Note that ctypes is already part of Python versions 2.5 and newer).

Defaulting color scheme to 'NoColor'
Python 3.2.2 (default, Sep  4 2011, 09:51:08) [MSC v.1500 32 bit (Intel)]
Type "copyright", "credits" or "license" for more information.

IPython 0.11 -- An enhanced Interactive Python.
?         -> Introduction and overview of IPython's features.
%quickref -> Quick reference.
help      -> Python's own help system.
object?   -> Details about 'object', use 'object??' for extra details.

In [1]: from IPython.external.path import path as Path

In [2]: Path('c:\windows').isdir
Out[2]: `<function nt._isdir>`

In [3]: Path('c:\windows').isdir()
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
C:\Python32\Scripts\<ipython-input-3-e7e1cd734888> in <module>()
----> 1 Path('c:\windows').isdir()

TypeError: _isdir() takes exactly 1 argument (0 given)

In [4]:
@takluyver
IPython member

OK, tracked it down. For Python 3.2.2, the implementation of os.path.isdir on Windows has changed to a built-in function. It seems that when you assign a built-in function to a class variable (i.e. the class definition has isdir = os.path.isdir), it doesn't get bound to class instances. I think we'll just have to produce a tiny wrapper method for the Path class:

def isdir(s): return os.path.isdir(s)

@fperez
IPython member

Feel free to make that tiny change in the 'if py3:' branch of the platform-specific patches. Since this is only a problem for the specific case when isdir is being assigned to an object instance (a somewhat unusual case), it should be only used for the path.py file, not in general. In fact, the patch should be applied to path.py itself, so it doesn't pollute the rest of the code with a somewhat unidiomatic trick that's unnecessary in normal cases.

@takluyver
IPython member

I was planning to apply it in path.py. But I wonder if I should do it for all Python versions - I think it may have been introduced in a bugfix Python 3 update, so it's possible it might be backported to Python 2.7.

@minrk
IPython member

I would just replace the isdir=os.path.isdir with def isdir(self): return os.path.isdir(self), and probably follow suit with other similarly defined methods. It just seems like the assumption that you can assign os.path.foo as a method is not entirely safe.

I don't see any reason to make it conditional, other than adding complexity and maximizing the probability that we see the issue again.

@takluyver
IPython member

I think that approach is sound.

@fperez
IPython member
@takluyver
IPython member

I did much the same test, and I agree with you - it should be transparent whether a function is written in Python or C. I'll implement Min's suggestion, if no-one else is already doing it.

@fperez
IPython member

Great, thanks!

@takluyver takluyver added a commit to takluyver/ipython that referenced this issue Sep 16, 2011
@takluyver takluyver Wrap os.path functions in method calls
Some functions from os.path are now references to C functions (e.g. isdir on Windows). This breaks the path module, because compiled functions do not get bound to an object instance. All os.path functions have been wrapped in method calls, out of general caution.

Closes gh-737
fc05f37
@takluyver
IPython member

Done and dusted. Forwarded the changes upstream too: https://github.com/dottedmag/path.py/pull/11

@fperez
IPython member

Great, good karma for pushing upstream. Thanks!

@mattvonrocketstein mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this issue Nov 3, 2014
@takluyver takluyver Wrap os.path functions in method calls
Some functions from os.path are now references to C functions (e.g. isdir on Windows). This breaks the path module, because compiled functions do not get bound to an object instance. All os.path functions have been wrapped in method calls, out of general caution.

Closes gh-737
7ebfa43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment