-
Notifications
You must be signed in to change notification settings - Fork 667
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
Refactor code to avoid use of rich library #4396
Conversation
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.
Some nitpicky stuff, looks great tho. Sorry about the late review,
# Regex to find opening tags and their content | ||
tag_pattern = re.compile(r"\[([\w\.]+)(?:=(.*?))?\]|\[/\]") | ||
|
||
def replace_bb_links(text: str) -> str: |
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.
Can you get rid of these nested functions? It's really hard to grok.
|
||
# Replace matches with HTML <a> tags | ||
|
||
def replacement(match: re.Match[str]) -> str: |
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.
3 deep?
print(self.render(data), end=end, file=file or self._file, flush=flush) | ||
|
||
def render(self, text: str) -> str: | ||
"""Parses a string containing nested BBCode with a generic block terminator ([/]).""" |
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.
Please add params to docstrings in google format :)
class AnsiStyle(PlainStyle): | ||
"""Theme.""" | ||
|
||
@dataclass |
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.
This is atypical, was there a need to nest this?
file: TextIO | None = None, | ||
flush: bool = False, | ||
) -> None: | ||
"""Internal print implementation.""" |
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.
Can you comment why this was necessary on the docstring?
"success": (style.success, style.normal), | ||
} | ||
# Regex to find opening tags and their content | ||
tag_pattern = re.compile(r"\[([\w\.]+)(?:=(.*?))?\]|\[/\]") |
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.
I've found putting the regex and the use of in it's own fn to make testing easier and demostrate what it does in the unit tests.
DIM = "\033[1;30m" | ||
BOLD_CYAN = "\033[1;36m" | ||
|
||
warning = "\033[33m" # yellow |
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.
Can this ref the constant in ANSI?
BOLD_CYAN = "\033[1;36m" | ||
|
||
warning = "\033[33m" # yellow | ||
error = ANSI.RED # "\033[31m" # red |
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.
Comment here probably not needed
Related: https://issues.redhat.com/browse/AAP-36125