Witty Wisterias #12
Witty Wisterias #12
Conversation
Setup: - Workflow - Pre-Commit - Project Structure - Linting/Formatting/Checking
- Add review.yaml Should work as expected to Enforce at least one Pull Request Review before Merging
- Remove Bot Comment is Approval is present - Make sure Action succeeds if approval is found - Move all to one Job for better Clarity in PR
- Check for Draft PRs - Approval Output Flag Check works now - Better Commenting/Naming
Fixes WittyWisterias/WittyWisterias#6 Probably not that space-efficient (yet), but good for now.
Define message format (Task 27)
- Raises Error if File Size exceeds 20mb - Chooses the most efficient rectangle for image dimensions (rather than a square) for space efficiency - Move Constant Terms to Class Values for better readability Co-Authored-By: Erri4 <118684880+erri4@users.noreply.github.com>
…s an error. NOTE: check line 108. the function returns None but the docstring says it returns the url.
… alone, not with Union
the search url isnt actually a constant, but a function, for dynamic search queries
- Implemented UserID Generation - Implemented Public Key Encryption - Implemented Digital Key Signing
- OCR using https://freeocr.ai/ - Text2Image using https://pollinations.ai/
This could be improved with a previous message updater, but its good for now and probably this CodeJam.
janine9vn
left a comment
janine9vn
left a comment
There was a problem hiding this comment.
The Overall Pictures
This is a very fun project with a unique take and implementation on the theme. There is a small bug or two that impact the user experience, but overall getting connected and sending messages back and forth works well. It was delightful seeing it in action. I have some comments on the code decisions, but none are project breaking or a major deal. I think your project structure was well thoughtout and made it easier to review. Very nicely done!
README & Documentation
The README is well done and has all the critical components such as a succinct description of the project, how to install and run the project, the features of the project and what the project looks like in action.
Many of the files, particularly the backend ones, tended to be over commented. Unless the code is particularly tricky or unintuitive, comments should not describe what is happening. The code should be clear enough from the function and variable names to discern what is happening. Instead, comments should provide additional and necessary context to help others (including future you) understand why the code is the way it is. If your comment does not provide necessary context that cannot be gained from reading the code itself, then the comment isn't necessary.
Commits
The commit messages are good quality and I don't have many notes there. A message here or there could be improved such as "bug fixes" can include a message body that's more descriptive of what the bug is and how it was fixed for more context. Imperative mode is used consistently throughout. The commits are of good size too. There's ocassionally a fairly large one, but that's sometimes the nature of it if it refactors.
Code Quality
There is a significant amount of usage of classes that contain only @staticmethods. This is largely unnecessary in Python and, in my opinion, should be avoided in most cases. The module level namespaces provides enough context that a class containing only static methods ends up making things more verbose than they need to be. As an example:
from backend.backend import Backend
Backend.do_thing()can be simplified to
from backend import backend
backend.do_thing()| Returns: | ||
| list[MessageFormat]: A list of MessageFormat objects reconstructed from the decoded data. |
There was a problem hiding this comment.
I don't believe this return is quite correct based on your type hints and a potential early return in your function.
| ) | ||
|
|
||
|
|
||
| class Backend: |
There was a problem hiding this comment.
This class is entirely static methods, which doesn't provide a lot of usefulness except to provide context elsewhere that this is all related to the backend. That same context can be achieved by having them as module level functions. Then in other files you would be able to do:
from backend import backend
backend.decode(...)Otherwise you end up with slightly odd and repetitive imports such as:
from backend.backend import Backend
Backend.decode(...)I also wouldn't use the term "Base class" in this context since it implies that this class is intended to be inherited, which it isn't.
| # Convert each MessageFormat object to JSON | ||
| upload_stack.message_stack = [ | ||
| message.to_json() for message in upload_stack.message_stack if isinstance(message, MessageFormat) | ||
| ] | ||
| # Serialize the list to JSON | ||
| json_stack = json.dumps(asdict(upload_stack)) | ||
| # Compress the JSON string | ||
| compressed_stack = zlib.compress(json_stack.encode("utf-8")) | ||
| # Encode to base64 for safe transmission | ||
| return base64.b64encode(compressed_stack).decode("utf-8") |
There was a problem hiding this comment.
I'll mention this once here but it does apply to other sections of code in this file.
In the vast majority of cases comments should not explain what the code is doing--the code should be fairly clear on that front. Comments, instead, should primarily be used to provide additional context about why a particular approach was taken, or if it's a tricky piece of code then it explains how the line works.
The code written here is straight-forward, has good variable names, and nothing tricky is going on. So the comments here are not necessary and add clutter rather than helpful context.
| profile_image_stack: dict[str, str] = field(default_factory=dict) | ||
| verify_keys_stack: dict[str, str] = field(default_factory=dict) | ||
| public_keys_stack: dict[str, str] = field(default_factory=dict) | ||
| message_stack: list[MessageFormat | str] = field(default_factory=list) |
There was a problem hiding this comment.
More information on these attributes and what they're intended to store would be helpful. It's not clear exactly what the public_keys and verify_keys are supposed to be used for. Based on other information later on in this file the keys are the user ids with the values containing what the variable name states, but that doesn't tell me a lot about their purpose. More details about this, likely in the class definition or in the from_json staticmethod detailing what the data arg should unpack to, would be helpful.
| if not ( | ||
| message.sender_id | ||
| and message.receiver_id | ||
| and message.event_type | ||
| and message.content | ||
| and message.timestamp | ||
| and message.own_public_key | ||
| and message.receiver_public_key | ||
| and message.private_key |
There was a problem hiding this comment.
This can be ever so slightly simplified to an if not all(...) pattern.
| # Getting the Response data | ||
| page_data = response_json.get("data", {}).get("response", {}) | ||
| # Returning the extracted Text | ||
| extracted_text: str = page_data.get("natural_text", "No text found.") |
There was a problem hiding this comment.
There is a bug here. The value associated with the key natural_text can be None. Because a valid key was found, the "No text found." never gets used.
Either the page_data response should be altered or there should be a check underneath this line similar to:
if extracted_text is None:
extracted_text = "No text found."Otherwise there is no feedback to the user that text was not found and makes it seem like the chat app is broken vs the image recognition not being able to identify text.
| self.messages.append(private_message) | ||
|
|
||
| # Wait for 5 seconds before checking for new messages again to avoid excessive load | ||
| await asyncio.sleep(5) |
There was a problem hiding this comment.
This might be worth adding as a configurable variable in case someone wants to increase or decrease when to check for new messages based on their server load.
| case "": | ||
| pass |
There was a problem hiding this comment.
If you want a true fallback case for match-case you should use the wildcard _.
Also I think I would rather see a more straightforward if-statement chain here. While a valid and working use case of pattern matching I don't think it gets you much benefit as is.
You make it a bit better by combining some of the cases with the same actions:
case "Signing Message..." | "Pushing Signed Message...":
await asyncio.sleep(0.3)| async def capture_loop(self) -> None: | ||
| """Continuously capture frames from the webcam and update the frame data.""" | ||
| if not webcam_cap or not webcam_cap.isOpened(): | ||
| raise RuntimeError("Cannot open webcam at index 0") |
There was a problem hiding this comment.
It may be worth making this a more graceful process where it disables the webcam feature rather than raising the error to stop the program or allow checking at other indexes.
| def main() -> None: | ||
| """Main entry point for the project.""" | ||
| return | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| main() |
There was a problem hiding this comment.
I'm not entirely sure what this code is doing here.
No description provided.