So I agree that this is a legitimate bug, and the patch does appear to fix the
problem, at least in the use case given.  I'm just not sure it does it in the
right way.

I'm confused by a few things in the original code.  Looking at the docstring
for destroyer() we see:

    """Remove every .py[co] file associated to received .py file.

    :param magic_tag: if None, removes __pycache__ directories,
        if False, removes python3.1's .pyc files only,
        otherwise removes given magic tag from __pycache__ directory
    :type magic_tag: None or False or str"""

Dmitry's patch changes this documented behavior because when magic_tag=None,
it will *not* remove the __pycache__ directory if that happens to be
/usr/lib/python3/dist-packages.  So if the patch is accepted, the docstring at
least has to be updated.

(Aside: if you really wanted to remove the entire __pycache__ directory and
everything in it, I wonder why one call to shutil.rmtree() wasn't used
instead?)

(Aside2: imp.cache_from_source() would give you the tagged .pyc file to
remove, but i guess of course the problem is that the version of Python 3
running py3clean won't necessarily match the version of pyc files you want to
clean up.  Still, I might use imp.cache_from_source() first, then split the
basename and substitute for the tags, or glob for the tags part of the
resulting file name.)

destroyer()'s documented behavior only makes sense when you're dealing with a
private package.  Otherwise, for public packages, it should never be correct
to remove the __pycache__ directory and all its contents.  An empty
/usr/lib/python3/dist-packages/__pycache__ shouldn't ever hurt.

Unfortunately, py3clean doesn't know whether it's dealing with a private or
public package.  I don't think `-p package` gives it enough information,
though perhaps there's a way to find out from the package name that I'm
missing.

The other thing I'd do is instead of hard-coding python3/dist-packages, I'd
probably compare the resulting directory against the list given by
site.getsitepackages():

>>> import site
>>> site.getsitepackages()
['/usr/local/lib/python3.2/dist-packages', '/usr/lib/python3/dist-packages', 
'/usr/lib/python3.2/dist-packages', '/usr/lib/dist-python']

If the directory being cleaned were any of these, then don't empty them out.

So I guess if no one else has any other suggestions, then the following
changes should be made to the patch, after which it could be applied:

* compare against site.getsitepackages() for directories that should not be
  removed.
* update the docstring to indicate that site package directories are not
  removed even if magic_tag is None

Attachment: signature.asc
Description: PGP signature

Reply via email to