[cli] Add prompt command for CLI logger. #9897
Conversation
|
Can one of the admins verify this patch? |
|
Test FAILed. |
There was a problem hiding this comment.
I think this is a great addition! I left some comments, most very small changes.
One thing I want to add is that confirm currently aligns user input, so that it starts at the same horizontal position without re-printing the message (it just prints a bunch of spaces in place of the message). I'm not sure if prompt should do the same, but it's certainly worth noting.
python/ray/autoscaler/cli_logger.py
Outdated
|
|
||
| complete_str = cf.underlined(msg + ":") + " " | ||
|
|
||
| self._print(complete_str, linefeed=False) |
There was a problem hiding this comment.
I see we need linefeed=False here so we couldn't just use print. The downside is that we have to call _format_msg(msg, *args, **kwargs) ourselves since _print does not format the message it prints.
python/ray/autoscaler/cli_logger.py
Outdated
| ans = ans.strip() | ||
| except KeyboardInterrupt: | ||
| self.newline() | ||
| res = default |
There was a problem hiding this comment.
default is undefined as far as I can see. confirm gets it from the _default keyword argument.
It might be a good idea to drop defaults completely, by returning "" both when the user presses "enter" with no input and when the user "^C"s.
Otherwise, we should display the default value somehow, but very long defaults will be a problem.
P.S.
I am actually leaning towards having an optional _default parameter, so if there is a default with prompt we don't forget to print it, but if there is not one we should keep asking until we get user input.
There was a problem hiding this comment.
I left this as is. I don't feel strongly about this, but in a lot of cases, validating the input will be more complex than just checking if it's an empty string or not (e.g., maybe it needs to be an int or something), so either we would need to pass in a lambda for validation or we should do the validation outside of the prompt function. So I left it as is for now.
|
Test FAILed. |
rkooo567
added
@author-action-required
richardliaw
added
the
@author-action-required
richardliaw
changed the title
Similar to cli_logger.confirm, but instead this allows arbitrary string answers. I haven't touched this class before, so this should be reviewed carefully. Also, it probably could be made more colorful.