-
Notifications
You must be signed in to change notification settings - Fork 602
Feat: frontend update #1188
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
base: main
Are you sure you want to change the base?
Feat: frontend update #1188
Conversation
ValbuenaVC
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! At the moment I can only think of stylistic changes. It's great to have this feature (and the cool ASCII art)
| def format_scenario_info(*, scenario_info: dict[str, Any]) -> None: | ||
| """ | ||
| Print formatted information about a scenario. | ||
|
|
||
| Args: | ||
| scenario_info: Dictionary containing scenario information. | ||
| """ | ||
| _print_header(text=scenario_info["name"]) | ||
| print(f" Class: {scenario_info['class_name']}") | ||
|
|
||
| description = scenario_info.get("description", "") | ||
| if description: | ||
| print(" Description:") | ||
| print(_format_wrapped_text(text=description, indent=" ")) | ||
|
|
||
| if scenario_info.get("aggregate_strategies"): | ||
| agg_strategies = scenario_info["aggregate_strategies"] | ||
| print(" Aggregate Strategies:") | ||
| formatted = _format_wrapped_text(text=", ".join(agg_strategies), indent=" - ") | ||
| print(formatted) | ||
|
|
||
| if scenario_info.get("all_strategies"): | ||
| strategies = scenario_info["all_strategies"] | ||
| print(f" Available Strategies ({len(strategies)}):") | ||
| formatted = _format_wrapped_text(text=", ".join(strategies), indent=" ") | ||
| print(formatted) | ||
|
|
||
| if scenario_info.get("default_strategy"): | ||
| print(f" Default Strategy: {scenario_info['default_strategy']}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor stylistic choice, but these functions could take kwargs since that's already a dictionary
| def format_scenario_info(*, scenario_info: dict[str, Any]) -> None: | |
| """ | |
| Print formatted information about a scenario. | |
| Args: | |
| scenario_info: Dictionary containing scenario information. | |
| """ | |
| _print_header(text=scenario_info["name"]) | |
| print(f" Class: {scenario_info['class_name']}") | |
| description = scenario_info.get("description", "") | |
| if description: | |
| print(" Description:") | |
| print(_format_wrapped_text(text=description, indent=" ")) | |
| if scenario_info.get("aggregate_strategies"): | |
| agg_strategies = scenario_info["aggregate_strategies"] | |
| print(" Aggregate Strategies:") | |
| formatted = _format_wrapped_text(text=", ".join(agg_strategies), indent=" - ") | |
| print(formatted) | |
| if scenario_info.get("all_strategies"): | |
| strategies = scenario_info["all_strategies"] | |
| print(f" Available Strategies ({len(strategies)}):") | |
| formatted = _format_wrapped_text(text=", ".join(strategies), indent=" ") | |
| print(formatted) | |
| if scenario_info.get("default_strategy"): | |
| print(f" Default Strategy: {scenario_info['default_strategy']}") | |
| def format_scenario_info(**kwargs) -> None: | |
| """ | |
| Print formatted information about a scenario. | |
| Args: | |
| kwargs: Dictionary containing scenario information. | |
| """ | |
| _print_header(text=kwargs["name"]) | |
| print(f" Class: {kwargs['class_name']}") | |
| description = kwargs.get("description", "") | |
| if description: | |
| print(" Description:") | |
| print(_format_wrapped_text(text=description, indent=" ")) | |
| if kwargs.get("aggregate_strategies"): | |
| agg_strategies = kwargs["aggregate_strategies"] | |
| print(" Aggregate Strategies:") | |
| formatted = _format_wrapped_text(text=", ".join(agg_strategies), indent=" - ") | |
| print(formatted) | |
| if kwargs.get("all_strategies"): | |
| strategies = kwargs["all_strategies"] | |
| print(f" Available Strategies ({len(strategies)}):") | |
| formatted = _format_wrapped_text(text=", ".join(strategies), indent=" ") | |
| print(formatted) | |
| if kwargs.get("default_strategy"): | |
| print(f" Default Strategy: {kwargs['default_strategy']}") |
| ValueError: If value is not a valid integer or violates constraints. | ||
| """ | ||
| try: | ||
| int_value = int(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to add stronger parsing? There may be some unexpected behaviors, e.g. if value = True, then int(value) == 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type annotation should be enough to constrain value to str anyway but I'm wondering if there's an edge case where this would break
| return int_value | ||
|
|
||
|
|
||
| def _argparse_validator(validator_func): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| def _argparse_validator(validator_func): | |
| def _argparse_validator(validator_func: Callable): |
| - Reuse the same validation logic in both argparse and non-argparse contexts | ||
|
|
||
| Args: | ||
| validator_func: Function that raises ValueError on invalid input. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| validator_func: Function that raises ValueError on invalid input. | |
| validator_func (Callable): Function that raises ValueError on invalid input. |
functools has some utilities that might help here, but I'm not too familiar
| import logging | ||
| import sys | ||
| from pathlib import Path | ||
| from typing import TYPE_CHECKING, Any, Optional, Sequence |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| from typing import TYPE_CHECKING, Any, Optional, Sequence | |
| from typing import TYPE_CHECKING, Any, Callable, Optional, Sequence |
Tests updated. Docs incorporated.