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 does not normalize paths #586

Closed aaronhendry closed 3 weeks ago

aaronhendry commented 3 weeks ago

Consider the following code:

from pprint import pprint

import inquirer
import os

if __name__ == '__main__':
    questions = [
        inquirer.Path(name='test', message="Enter valid path", path_type=inquirer.Path.DIRECTORY, exists=True,
                      normalize_to_absolute_path=True, default="~/")
    ]
    resp = inquirer.prompt(questions)
    pprint(resp)

Run this and hit enter to accept the default.

The expected output from this would be {'test': '/home/username/'}, however instead we get {'test': '~/'}. Perhaps I am misunderstanding the use-case of the normalize_to_absolute_path flag, but I was assuming it would normalize the output.

I believe the reason for this is because the validate method currently doesn't have any way of modifying the passed path. It normalizes it in order to validate it, but the original input (~/ in this case) is the value that is returned to the user. I think the "easy" solution here would be to allow the validate function of each question to return a value, such that the value could be modified as needed.

Cube707 commented 3 weeks ago

hm, combined with your other issue I think it would make sense to rewrite the whole thing to use pathlib.Path.

I am just debating if the returntype should change or if/how I hide that behind a flag.

aaronhendry commented 3 weeks ago

I had that thought too, but I didn't want to put in a pull request completely rewriting it, nor suggest that the best course of action was a whole lot of work for you (or someone, at least).

I think part of the problem is that your validate method is doing double-duty, modifying the reply at the same time that it is validating it. It might make more sense to add a preprocessor function to the root Question class that is noop by default, and then override it in the Path class. Something like:

    # In the Question class
    def preprocess(self, current):
        return current
...
    # In the Path class
    @override
    def preprocess(self, current):
        if not current:
            return current
        return self.normalize_value(current)

That would have fairly minimal impact on the code as it is, I think. I've not spent too much time thinking about it though, so grain of salt and all that.

Cube707 commented 3 weeks ago

I arrived at the conclusion that inquire should always return the original input as given by the user, meaning responsibilities like normalizing the path should be up to the caller.

I have no idea what normalize_to_absolute_path was originally meant to do