Skip to content
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

Accept pathlib paths across the board #1311

Open
Jwink3101 opened this issue Jan 8, 2021 · 6 comments
Open

Accept pathlib paths across the board #1311

Jwink3101 opened this issue Jan 8, 2021 · 6 comments

Comments

@Jwink3101
Copy link

This is an issue in preparation for a patch I will be trying to make.

I want to update Bottle to accept pathlib.Path paths across the board. My plan is to do this in a backwards COMPATIBLE way by adding something like str(inputfile) so that if you pass a native string or unicode, nothing happens but if you pass a pathlib object, it gets converted.

First question: Does this plan make sense? I use this in my tools so that I don't have to think about what is passed and it is explicitly converted

Second question: Where should I be looking? I think the goal should be as much coverage as possible but I would take better over perfect*. Off the top of my head:

  • lookup paths for SimpleTemplate
  • Upload handling
  • (maybe) config options

Anywhere else?

Other thoughts?

*It took the python standard library a few major versions to transition too.

@oz123
Copy link
Contributor

oz123 commented Jan 8, 2021

Can you post an example snippet of how this change would look?

@defnull
Copy link
Member

defnull commented Jan 8, 2021

Sounds fine, but instead of str() I'd suggest to add a (private) _strpath() function that raises TypeError for value types that make no sense. Using str() would render sanity checks in other libraries useless, as for example None would be passed on as a string.

@Jwink3101
Copy link
Author

Jwink3101 commented Jan 8, 2021

re Private method

It makes sense to use a private _strpath() method, but there are some complexities in that. The main issue is that (for some reason) pathlib does not offer its own method to turn a path into a string. The method suggested method in the docs is to use str(). So you cannot look for a certain attribute to call to make it a string. So without directly importing it, you can't do isinstance

This is also further complicated by the fact that (as I found in trying to answer oz123's question below), there are instance checks for base-string. So instead I propose the following. Note that this is NOT done in the if py3k conditional to account for backports

try:
    from pathlib import Path # 3.4+ or backport in python2
except ImportError:
    class Path(object):
        pass # dummy class. We never call it. Only used for instance checks
...

def _strpath(path):
    """Convert pathlib.Path objects"""
    if isinstance(path,Path):
        return unicode(path)
    return path 

(I use unicode to again support pyhton2)

Of course, the other option is to import pathlib and just do an instance check but that would break python2. I've seen in other PRs that you are hesitant to break python2 "just because". This is a reason to but it is also pretty minor. And then option 2.5 which is leave this open and don't make changes until we're already broken python2

re example:

Here is an example, but it actually shows the complexity. This is in the FileUpload class

    def save(self, destination, overwrite=False, chunk_size=2 ** 16):
        """ Save file to disk or copy its content to an open file(-like) object.
            If *destination* is a directory, :attr:`filename` is added to the
            path. Existing files are not overwritten by default (IOError).

            :param destination: File path, directory, pathlib.Path, or file(-like) object.
            :param overwrite: If True, replace existing files. (default: False)
            :param chunk_size: Bytes to read at a time. (default: 64kb)
        """
        if isinstance(destination, (basestring,Path)):  # Except file-likes here
            destination = _strpath(destination)
            if os.path.isdir(destination):
                destination = os.path.join(destination, self.filename)
            if not overwrite and os.path.exists(destination):
                raise IOError('File exists.')
            with open(destination, 'wb') as fp:
                self._copy_file(fp, chunk_size)
        else:
            self._copy_file(destination, chunk_size)

So not too bad I guess.

I just need to find all of the examples where this happens.

@Marrin
Copy link

Marrin commented Jan 9, 2021

Shoudn't be the instance check against os.PathLike and/or make a test against the __fspath__ attribute.

@Jwink3101
Copy link
Author

That's new in 3.6. So it won't support backported pathlib (which I am fine with) and older python.

What is the minimum Bottle should support? And I again ask, is this the issue to break support? Probably not.

@iamgodot
Copy link

Is this still in progress? I'm trying to achieve the same thing, also maybe something like str(path.resolve()) would do better.

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

No branches or pull requests

5 participants