Laudatory Larkspurs#16
Conversation
Merge GUI features to main
# Conflicts: # src/commands/load_image.py # src/terminal.py
added some exclusions in toml as well
# Conflicts: # src/utils/color.py
janine9vn
left a comment
janine9vn
left a comment
There was a problem hiding this comment.
The Overall Picture
This was a very fun project to test out. Your overall project structure and organization is nicely done. It makes it easy to review and reason about the code. Your project functioned really well and everything was intuitive once you read through the basic commands. Things worked seamlessly which is very impressive given the time constraints. Overall, very well done!
README & Documentation
The README and Documentation are excellent. I think an area of improvement could be adding some example commands for each major feature to try out, or showcase one of the more complex commands. There are a few spots in the code where I think some comments are redundant and some other parts where an additional comment or two would be helpful. Typehints are well done. I think the docstrings on some of the busier functions could be improved a bit and include more information, but otherwise the class docstrings are well done and no other complaints.
Commits
The commits overall look good in terms of scope and messages. There are a few where the messages could be improved though. For instance "bugfixes" or "fixed ping" could be improved by either calling out specific bugs in the commit title or in the body if you need more space to detail things.
Code Quality
Overall the code quality is of a good level. There are a few places where dunders are used when there's more straight-forward syntax that can be used instead. Some formatting every-so-often was a bit odd and I think could be slightly improved. Overall your implementation is very class-heavy, but that's not a bad thing and makes sense with what your project deals with. Some of your functions were heavily indicated as "private" with the leading underscore, but across the project that was inconsistent.
| from terminal import Terminal | ||
|
|
||
|
|
||
| class BaseCommand: |
There was a problem hiding this comment.
Have you looked into using the @abc.abstractmethod for this to require functions to be overridden by subclasses instead of the manual raising of the NotImplementedError?
Based on the docstrings for the functions it seems like that may be what you want.
|
|
||
| match len(args): | ||
| case 0: | ||
| return " " + str(terminal.background_color.r) |
There was a problem hiding this comment.
An f-string would be good to use here: return f" {terminal.background_color.r}"
| if not (args[2].isdigit()): | ||
| terminal.output_error("Invalid radius.") | ||
| return False | ||
| rad = int(args[2]) | ||
| if rad < 0: | ||
| terminal.output_error("Radius cannot be negative.") | ||
| return False |
There was a problem hiding this comment.
Nice pre-emptive checking!
| display: flex; | ||
| flex-direction: column; | ||
| width: 100%; | ||
| height: 100%; |
There was a problem hiding this comment.
I would indent this a level so it doesnt look like args to the init directly. I did a double take to see if there was a misplace " before I realized what the intent was.
| if command_verb: | ||
| self.text += command_verb.group(1) | ||
|
|
||
| command_verb_span.text = command_verb.group(2) | ||
| self.append_child(command_verb_span) | ||
|
|
||
| command_end = text[len(command_verb.group(0)) :] | ||
| command_end_span = Element(tag_name="span") | ||
| command_end_span.text = command_end | ||
| self.append_child(command_end_span) |
There was a problem hiding this comment.
Either in the docstring or as a comment, some more context about the approach and the intent could be helpful.
Something like: If it's a command verb then we ...
| natural_width = self.image_element["naturalWidth"] | ||
| natural_height = self.image_element["naturalHeight"] | ||
| intrinsic_mouse_x = int( | ||
| ((event.clientX - self.image_element["offsetLeft"]) / self.image_element["clientWidth"]) * natural_width, |
There was a problem hiding this comment.
Small formatting thing, but I prefer this to be formatted like: ((event.ClientX - self.image_element["offsetLeft"])/self.image_element["clientWidth"])*natural_width
It's a bit easier to parse with the precedence of the operators and brings it closer into alignment with PEP8
| else: | ||
| print("Warning: TerminalGui has no Terminal instance assigned.") |
There was a problem hiding this comment.
Would this be worth raising an actual warning or error for?
| def set_value(self, value: str) -> None: | ||
| """Set the value of the text input.""" | ||
| self.text_input["value"] = value | ||
| command = re.search(r"^(\s*)(\S+\b)", value) |
There was a problem hiding this comment.
A comment here to help explain the intent of the regex would be helpful.
| match = re.search(rf"^(\d+){sep}(\d+){sep}(\d+)(?:{sep}(\d+))?$", color_string) or re.search( | ||
| rf"^rgba?\((\d+){sep}(\d+){sep}(\d+)(?:{sep}(\d+))?\)$", | ||
| color_string, | ||
| ) |
There was a problem hiding this comment.
I would format this a bit different for easier readability, something like this so that the regexes are a bit easier to compare to see what they're doing:
match = re.search(
rf"^(\d+){sep}(\d+){sep}(\d+)(?:{sep}(\d+))?$",
color_string
) or re.search(
rf"^rgba?\((\d+){sep}(\d+){sep}(\d+)(?:{sep}(\d+))?\)$",
color_string,
)| return Color(int(r), int(g), int(b), a if a else 255) | ||
|
|
||
|
|
||
| colors = { |
There was a problem hiding this comment.
Are these pre-defined colors shown the user via the help command at some point? If not, that would be a good improvement to make.
6 participants
No description provided.