Skip to content

Support path-like objects for reading/writing #233

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 17, 2022

Conversation

mwtoews
Copy link
Contributor

@mwtoews mwtoews commented Feb 15, 2022

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

@karimbahgat
Copy link
Collaborator

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
Copy link
Collaborator

Alternatively just checking for the __fspath__ attribute/protocol directly?

@mwtoews
Copy link
Contributor Author

mwtoews commented Feb 23, 2022

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
Copy link
Collaborator

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
Copy link
Contributor Author

mwtoews commented Feb 24, 2022

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 karimbahgat merged commit 186933b into GeospatialPython:master Mar 17, 2022
@karimbahgat
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants