Top Level Walruses #10
Conversation
Co-authored-by: Noah Tanner <[email protected]>
There was a problem hiding this comment.
Code Quality
The code itself is very well done. In particular some of the extra work gone into the files in common.py is appreciated, to clearly define non-standard behavior and avoid messy hacks that would otherwise be necessary. The typehints are well done and consistent as well. The code is logical and (mostly) easy to follow and reason about.
Documentation
One aspect that was lacking in this project was documentation. Many functions did not have docstrings, and when they did, they often felt lacking. Some of the more dense code as well could have substantially benefitted from comments explaining what the intent is and some of the more technical details. I did not comment on every instance, but it was pretty much project-wide and did make this project a bit harder to review.
The README/presentation was well done. I appreciate the demo examples included and how everything was explained. The one aspect I think could be improved in general for the README, or in the code itself, is a more robust explanation of the different components of the Piet interpreter, generator, reader, runtime, and the common files. There's a lot going on there and could be benefitted from more words to help explain what's happening.
Commit Messages
The commit messages overall were good. Some could have been a bit more descriptive about what the commit itself accomplished. For instance "piet interpreter almost finished", could instead describe what was added to get the piet interpreter to an almost finished state. In general, for more substantial commits, I'm a fan of being more specific about what was added in the commit body.
Summary
This is a very impressive project, with a fully working interpreter and generator for Piet. The lack of documentation is a sore spot, but otherwise I don't have major comments. The organization and structure of the project as well is nicely done and logical. It made it easier to review. Additionally, I do appreciate the inclusion of tests. This code jam is usually a time crunch, so including tests is going a bit above and beyond.
A quick note...
Thank you to dawn and aboo for reviewing parts of this project and providing some of the comments I left.
| def generate( | ||
| input_path: Path, | ||
| output_path: Path, | ||
| cols: Optional[int] = None, |
There was a problem hiding this comment.
This is minor, but the slightly recommended way to do this typehint would be: int | None
There was a problem hiding this comment.
IIRC Optional is here because Typer does not support the new-style annotation type | None.
| print("\n") | ||
| print("Successfully generated a Piet program!") | ||
| print(f"Image saved to: {output_path}") | ||
| print(f"You can run it with: {command}") |
There was a problem hiding this comment.
This is a nice QoL feature for the user, that you show the command they can run.
| if input_path.exists(): | ||
| data = input_path.read_bytes() | ||
| else: | ||
| data = str(input_path).encode() | ||
| command = f'python -m {__package__} run "{output_path}"' | ||
| first_input = next(iter(input), "") | ||
| generator = ImageGenerator(input=first_input, step_limit=step_limit, debug=debug) | ||
| if recurse: | ||
| image = generator.generate_recursive(data, (v.encode() for v in input), recurse) | ||
| command += " --execute" | ||
| else: | ||
| key = first_input.encode() | ||
| if key: | ||
| key *= len(data) // len(key) + 1 | ||
| command += f' --input "{first_input}"' | ||
| image = generator.generate_image(data, key, cols) |
There was a problem hiding this comment.
I think this function could use a docstring or some comments in this area about what's happening. It's doing a fair bit and not all of it necessarily straightforward.
| T = TypeVar("T") | ||
|
|
||
|
|
||
| class SelfExpandingList(list[T]): |
There was a problem hiding this comment.
This class is doing quite a bit. It is in desperate need of some explainer comments about what its purpose is and what it is trying to do.
You can piece together what it is doing and its purpose after a bit, but comments would go a long way.
There was a problem hiding this comment.
To be honest, this was more of a last-ditch attempt to facilitate a dynamically sized 2-dimentional array without lengthy blocks of logic and append calls.
| # lightness hue | ||
| # v v | ||
| PIET_COLORS: tuple[tuple[Color, ...], ...] = ( |
There was a problem hiding this comment.
Cute comment and very helpful
| height, width = len(colors), len(colors[0]) | ||
| smallest = height | ||
|
|
||
| rsf = 0 |
There was a problem hiding this comment.
What is rsf? It is unclear in this section what it is intended to represent.
| for y in range(height): | ||
| for x in range(width): | ||
| color_ints.append(int(colors[y][x])) |
There was a problem hiding this comment.
This can be replaced with some functions from itertools:
import itertools
...
for color in itertools.chain.from_iterable(colors):
color_ints.append(int(color))or, depending on what width is, just itertools.product
| else: | ||
| if rsf < smallest: | ||
| smallest = rsf | ||
| pixel = colors[y][x] | ||
| rsf = 1 |
There was a problem hiding this comment.
This can be simplified into an elif
| def __repr__(self) -> str: | ||
| return ( | ||
| f"StepTrace({self.iteration:0>4}," | ||
| f" instruction={self.instruction.__name__}," | ||
| f" position={self.position}," | ||
| f" did_flip={self.did_flip}," | ||
| f" last_codel={self.last_codel}," | ||
| f" current_codel={self.current_codel})" | ||
| ) |
There was a problem hiding this comment.
Very nice usage of a repr here!
| class PointerDirection(Enum): | ||
| RIGHT = OrderedPair(0, 1) | ||
| DOWN = OrderedPair(1, 0) | ||
| LEFT = OrderedPair(0, -1) | ||
| UP = OrderedPair(-1, 0) |
There was a problem hiding this comment.
This is a nice way of handling directions.
No description provided.