LibraryOfCongress / bagit-python

Work with BagIt packages from Python.
http://libraryofcongress.github.io/bagit-python
216 stars 85 forks source link

self.path shouldn't be None? #166

Open fkloft opened 1 year ago

fkloft commented 1 year ago

From bagit.py:

class Bag(object):
    # [...]
    def __init__(self, path=None):
        # [...]
        self.path = abspath(path)
        if path:
            # if path ends in a path separator, strip it off
            if path[-1] == os.sep:
                self.path = path[:-1]
            self._open()

The path argument defaults to None, but either way, the __init__ method probably doesn't do what's intended.

If path is None, the call to abspath (from os.path) fails with a TypeError. If path is not None, self.path is possibly overridden by self.path = path[:-1]. It should be noted that os.path.abspath already removes the trailing path separator, so the check shouldn't be necessary. If there was a trailing path separator, self.path suddenly is not an absolute path any more.

os.path.abspath only leaves the trailing separator if path == "/". Although I guess there should be no use case where you would want to create a bag at the filesystem root directory.

I noticed this when I was passing a pathlib.Path object to Bag(). I assumed it would work correctly as os.path.abspath can handle PathLike objects, but the additional check for a trailing path separator broke because pathlib.Path objects are not subscriptable.

fkloft commented 1 year ago

Is there even a use case for self.path to be None?

I would guess no; to create new bags, there's make_bag().