GeospatialPython / pyshp

This library reads and writes ESRI Shapefiles in pure Python.
MIT License
1.09k stars 259 forks source link

Support path-like objects for reading/writing #233

Closed mwtoews closed 2 years ago

mwtoews commented 2 years ago

This PR enables support for reading/writing with path-like objects, e.g. pathlib or the tmpdir pytest fixture.

karimbahgat commented 2 years ago

Thanks once again, this looks great. Looks like you predicted #234 by two days!

I'm wondering if it might work to simplify "pathlike_obj()" to a single try-except forcing to str as suggested in #334, without all the version checks and class inspections? That way the code is cleaner, we avoid possible unforeseen issues, and support any path like objects that can be converted to str. Seems like Path objects can be forced to string, but not sure about pytest tmpdir objects?

karimbahgat commented 2 years ago

Alternatively just checking for the __fspath__ attribute/protocol directly?

mwtoews commented 2 years ago

Good question, there are a few ways to detect if objects support a path-like protocol. For Python 3.6+ it's just isinstance(path, os.PathLike), as is already done. But for Python 3.5 and lower, there was no __fspath__ attribute/protocol, which is why a loose test "path" in path.__class__.__name__.lower() is done. I can't think of any other way, but am open to other suggestions.

karimbahgat commented 2 years ago

After some digging, I'm thinking a simple check for fspath is sufficient for those that implement it, and for those that don't implement it (eg pre 3.6) simply try str(path), which would work for pathlib pre-3.6 and also appears to be supported by pytest tmpdir. This way the logic is simple and we don't have to worry about versions etc in pathlike_obj(), so something like:

def pathlike_obj(path):
    if is_string(path):
        return path
    elif hasattr(path, "__fspath__"):
        return path.__fspath__()
    else:
        try:
            return str(path)
        except:
            return path

I would prefer this approach and also think it has the least likelihood of breaking anything for existing users or future changes. What do you think? If you agree, could you amend the PR to change the pathlike_obj function to the above?

mwtoews commented 2 years ago

Thanks for the better suggestion! However, I've kept the python version split, since os.fsdecode is the preferred method to convert these objects to str rather than str() (even though it'll be the same in most cases). Also, I've discovered that pytest requires pathlib2 for older Python versions, which has the same functionality as the pathlib that is part of Python3, therefore no tests are skipped.

karimbahgat commented 2 years ago

Sounds good, thanks! Should be included in next version :)