magmax / python-inquirer

A collection of common interactive command line user interfaces, based on Inquirer.js (https://github.com/SBoudrias/Inquirer.js/)
MIT License
981 stars 98 forks source link

inquirer.Path rejects valid default paths #585

Closed aaronhendry closed 3 weeks ago

aaronhendry commented 3 weeks ago

The inquirer.Path question currently does not support providing a default under the following conditions:

The reason for this is due to this line

https://github.com/magmax/python-inquirer/blob/7ce0a8ab7d5122f140e1079b69ef40b533ee2560/src/inquirer/questions.py#L287

The issue is that os.abspath removes the trailing slash from directories when converting to an absolute path. This means that a valid directory like /usr/bin/ is converted to /usr/bin, which then fails the os.path.basename(current) != "" validation check.

It's not terribly difficult to fix this, for instance:

value = os.path.abspath(value) + (os.sep if value[-1] == os.sep else "")

It's probably better to change the way this is checked, however, as there is no reason that an existing directory should need the trailing slash to make it a valid directory. You could change the existence check to

if self._exists is None:
    if os.path.exists(current):
        if not os.path.isdir(current):
            raise errors.ValidationError(current)
    elif os.path.basename(current) != "":
        raise errors.ValidationError(current)

But that makes things a bit more complicated. I guess one possibility is to consider any non-existent path as a valid directory in potentia. Most python commands will accept paths without a trailing slash as a valid directory; for instance, as long as you have write permissions, pathlib.Path('/non/existent/path'.mkdir(parents=True) is perfectly valid. In this instance it would be perfectly fine for the only failure condition to be if the file exists and it's a file, not a directory.

Cube707 commented 3 weeks ago

I thought on this a little bit and the following makes the most sense to me:

If exists=true:

if exists=false:

And the additional argument: If you had to type a path into a form and it rejects your input because yo don't have a / I would get annoyed. So this should probably only be enforced if it is required to remove ambiguity.

open("foo","w").close()
inquirer.path("d", path_type=inquirer.Path.DIRECTORY)
> foo  # this should fail
> foo/ # this is accepted
> bar  # this is also acceptable