Revamp UI interactions #214
Conversation
|
Looks like only the |
|
This'll be why |
I'll do another targeted branch to fix this. |
|
I haven't looked at the details here yet. General idea look fine. This is what I'm thinking - there is probably a fair bit more we can/should do to improve. For example, tighten up typing to be more specific instead of many parameters/return values being strings. I don't want to get this improvement bogged down in trying to fix everything, but I don't want to shoot ourselves in the foot and complicate future improvements. In any case, I'll get in and review this as is. And hopefully none of that is an issue. |
|
|
||
| from typing import Callable, Any | ||
| from queue import Queue, Empty | ||
| from enum import StrEnum |
There was a problem hiding this comment.
StrEnum requires python >=3.11.
| return self.error_msg | ||
|
|
||
|
|
||
| class UiColour(StrEnum): |
There was a problem hiding this comment.
See above for import error
class UiColour(str, Enum):I believe there are some subtle differences betweeen this and StrEnum but I doubt they really affect the way this is being used. I think this mixin does all the magic interpret as string that we want.
https://docs.python.org/3/library/enum.html#enum.StrEnum
https://docs.python.org/3/library/enum.html#notes
| class Validator: | ||
| """ | ||
| Defines a validator object that can be used to validate user input. | ||
| """ | ||
|
|
||
| def __init__(self, func: Callable[[Any], bool], errror_msg: str = "Invalid input"): | ||
| """ | ||
| Args: | ||
| func (function): The function to validate the input | ||
| error_msg (str): The message to display if the input is invalid | ||
| """ | ||
| self.func = func | ||
| self.error_msg = errror_msg | ||
|
|
||
| def __call__(self, resp: Any) -> bool: | ||
| """ | ||
| Args: | ||
| resp (Any): The response to validate | ||
| Returns: | ||
| bool: True if the response is valid, False otherwise | ||
| """ | ||
| return self.func(resp) | ||
|
|
||
| def __str__(self) -> str: | ||
| return self.error_msg |
There was a problem hiding this comment.
Can use generics to improve matching types between call and init.
T = TypeVar("T")
class Validator(Generic[T]):
"""
Defines a validator object that can be used to validate user input.
"""
def __init__(self, func: Callable[[T], bool], errror_msg: str = "Invalid input"):
"""
Args:
func (function): The function to validate the input
error_msg (str): The message to display if the input is invalid
"""
self.func = func
self.error_msg = errror_msg
def __call__(self, resp: T) -> bool:
"""
Args:
resp (T): The response to validate
Returns:
bool: True if the response is valid, False otherwise
"""
return self.func(resp)
def __str__(self) -> str:
return self.error_msgAn untyped callable will count as Any as before
def hello(arg) ->bool:
...
v = Validator(hello)
v(1)
v("st")A typed callable will warn if the wrong type is passed in
def hello(arg:int) ->bool:
...
v = Validator(hello)
v(1)
v("st") # literal not assignable to int| GREY = "grey" | ||
|
|
||
|
|
||
| def _user_request_input(msg: str): |
There was a problem hiding this comment.
should annotate return as str if the public function that wraps this always expects this to return str
| def user_serial( | ||
| msg: str, | ||
| validator: Validator = _ten_digit_int_serial_v, | ||
| return_type: int | str = int, |
There was a problem hiding this comment.
breaks python 3.8
use
return_type: Union[Type[str], Type[int]] = int,Additionally can use typevars to match the input and the return type if you want to go overboard.
Does this give much more advantage over just always returning a string and then manually converting to an int if required? Or is it more of compatibility thing
| def _reformat_text(text_str, first_line_fill="", subsequent_line_fill=""): | ||
| lines = [] | ||
| _wrapper_initial_indent = wrapper.initial_indent | ||
| _wrapper_subsequent_indent = wrapper.subsequent_indent | ||
| wrapper.initial_indent = first_line_fill | ||
| wrapper.subsequent_indent = subsequent_line_fill | ||
| for ind, line in enumerate(text_str.splitlines()): | ||
| if ind != 0: | ||
| wrapper.initial_indent = subsequent_line_fill | ||
| lines.append(wrapper.fill(line)) | ||
| # reset the indents, calls to this method should not affect the global state | ||
| wrapper.initial_indent = _wrapper_initial_indent | ||
| wrapper.subsequent_indent = _wrapper_subsequent_indent | ||
| return "\n".join(lines) |
There was a problem hiding this comment.
Were there bugs with this? It looks like nothing uses the optional arguments anywhere.
It looks like this whole thing be replaced which removes the global shared state
def _reformat_text(text, first_line_fill="", subsequent_line_fill=""):
return textwrap.wrap(
text,
width=75,
break_long_words=False,
replace_whitespace=False,
initial_indent=first_line_fill,
subsequent_indent=subsequent_line_fill
)| for _ in range(attempts): | ||
| pub.sendMessage("UI_req_choices", msg=msg, q=q, choices=choices) | ||
| resp = q.get() | ||
| ret_val = target(resp, choices) |
There was a problem hiding this comment.
Check for target not None or remove default value?
|
|
||
| txt_fill = "!" * wrapper.width | ||
|
|
||
| # Hisotry window |
| # I don't think the result of this code ever gets used, nothing looking for "ABORT_FORCE" | ||
| # unpacks from a tuple. This can probably be deleted. |
There was a problem hiding this comment.
2 things in main look for this, I really don't understand what these are doing though. Doesn't make sense to me that we would inidicate a forced abort during a user input function
There was a problem hiding this comment.
Do you want to give reformat_text the same treatment you did in the cli?
3 participants
This PR addresses Issue #177 and Issue #213.
I've done a similar thing to the switching module and put the new _ui.py module at the fixate level and have only exposed the public functions.
To avoid having 2 different sets of functions in
cmd_line.pyandui_gui_qt.pyI moved the (duplicated) logic out of the UI files into ui.py and kept the original behaviour, tests were updated to deal with this change.Also added the ability to have colour on
user_info_importantthe line of!defaults to red, which will hopefully be a bit more eye catching.Have tested each of the functions manually. But should try on a real script or 2 before this gets merged if it does.