Drawing output result #10
Conversation
WalkthroughThe recent updates enhance the game's data handling and user interaction by refining validation, database interactions, and visual feedback. Notably, game results now undergo more thorough validation, and a significant new feature is the creation of visual game summaries. Database operations have been streamlined for efficiency, and code cleanliness has improved through the removal of unnecessary elements and better structuring. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
pyproject.tomlis excluded by:!**/*.toml
Files selected for processing (7)
- src/data_models/Game.py (2 hunks)
- src/data_models/Player.py (1 hunks)
- src/data_models/Record.py (1 hunks)
- src/db.py (4 hunks)
- src/handlers/game.py (1 hunks)
- src/handlers/save.py (2 hunks)
- src/services/draw_result_image.py (1 hunks)
Files skipped from review due to trivial changes (1)
- src/data_models/Player.py
Additional comments: 5
src/data_models/Game.py (1)
- 6-6: The addition of the
configimport statement is noted. Ensure that theconfigmodule is used appropriately within this file, particularly in thevalidate_resultsmethod whereconfig.GAME_POLL_OUTCOMESis referenced.src/db.py (2)
- 10-12: The refactor of the
aiosqlite.connectcall in theget_dbfunction improves readability by clearly specifying the connection timeout. This is a good practice for managing database connections, especially in a long-running application.- 19-19: The adjustments to indentation in the function signatures for
fetch_all,fetch_one,execute, and_get_cursorenhance the readability of the code. Consistent indentation is crucial for maintaining code readability and should be adhered to throughout the project.Also applies to: 29-29, 39-39, 56-56
src/services/draw_result_image.py (2)
- 33-40: The
svg2pngfunction usescairosvgto convert SVG strings to PNG bytes. This is a straightforward and effective way to handle the conversion. Ensure that theunsafe=Trueparameter is necessary and understood, as it allows execution of arbitrary URLs or file paths, which could introduce security vulnerabilities if the SVG content is not fully controlled or sanitized.Consider verifying the necessity of
unsafe=Trueand ensure that all SVG content processed by this function is trusted or sanitized to prevent potential security issues.
- 177-187: The
fix_python_wrong_svg_stringfunction corrects SVG string attributes for compatibility and prettifies the output. This is a useful post-processing step to ensure the generated SVGs are compatible with web standards. However, be mindful of the performance implications of string replacements and XML parsing for large SVGs. If performance becomes an issue, consider optimizing this function or preprocessing SVG templates to minimize the need for these corrections.
| if ( | ||
| "I'm Liberal Winner" | ||
| or "I'm Hitler Looser" | ||
| or "I'm Fascistic Looser" in outcomes | ||
| ): | ||
| return "LW" | ||
| if "I'm Fascistic Winner" in outcomes: | ||
| if ( | ||
| "I'm Fascistic Winner" | ||
| or "I'm Hitler Winner" | ||
| or "I'm Liberal Looser" in outcomes | ||
| ): |
There was a problem hiding this comment.
The logical condition in the validate_results method for determining "LW" and "FW" outcomes seems incorrect. The use of or in the condition will always evaluate to True before checking membership in outcomes, which is not the intended logic. This should be corrected to ensure that the method accurately validates game results.
- if ("I'm Liberal Winner" or "I'm Hitler Looser" or "I'm Fascistic Looser" in outcomes):
+ if any(outcome in outcomes for outcome in ["I'm Liberal Winner", "I'm Hitler Looser", "I'm Fascistic Looser"]):- if ("I'm Fascistic Winner" or "I'm Hitler Winner" or "I'm Liberal Looser" in outcomes):
+ if any(outcome in outcomes for outcome in ["I'm Fascistic Winner", "I'm Hitler Winner", "I'm Liberal Looser"]):| @@ -21,7 +21,7 @@ async def game(update: Update, context: ContextTypes.DEFAULT_TYPE) -> None: | |||
| ) | |||
|
|
|||
| # Save some info about the poll the bot_data for later use in receive_poll_answer | |||
| game_metadata = { | |||
| game_metadata = { # TODO write it to DB | |||
There was a problem hiding this comment.
The TODO comment about writing game_metadata to the database is a good reminder of future work that needs to be addressed. It's important to track this task outside of the codebase, perhaps in a project management tool or issue tracker, to ensure it doesn't get overlooked.
Would you like me to open a GitHub issue to track the implementation of writing game_metadata to the database?
src/data_models/Record.py
Outdated
| def get_team(self) -> Optional[Literal["Fascist", "Liberal"]]: | ||
| if ( | ||
| self.role == "CH" | ||
| or self.role == "DH" | ||
| or self.role == "FW" | ||
| or self.role == "FL" | ||
| or self.role == "HL" | ||
| ): | ||
| return "Fascist" | ||
| elif self.role == "LW" or self.role == "LL": | ||
| return "Liberal" | ||
| else: | ||
| return None |
There was a problem hiding this comment.
The get_team method correctly determines a player's team based on their role. However, consider using a dictionary mapping of roles to teams for a cleaner and more efficient implementation. This approach can simplify the method and make it easier to maintain, especially if new roles or teams are added in the future.
- if (self.role == "CH" or self.role == "DH" or self.role == "FW" or self.role == "FL" or self.role == "HL"):
+ role_to_team = {"CH": "Fascist", "DH": "Fascist", "FW": "Fascist", "FL": "Fascist", "HL": "Fascist", "LW": "Liberal", "LL": "Liberal"}
+ return role_to_team.get(self.role)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def get_team(self) -> Optional[Literal["Fascist", "Liberal"]]: | |
| if ( | |
| self.role == "CH" | |
| or self.role == "DH" | |
| or self.role == "FW" | |
| or self.role == "FL" | |
| or self.role == "HL" | |
| ): | |
| return "Fascist" | |
| elif self.role == "LW" or self.role == "LL": | |
| return "Liberal" | |
| else: | |
| return None | |
| def get_team(self) -> Optional[Literal["Fascist", "Liberal"]]: | |
| role_to_team = {"CH": "Fascist", "DH": "Fascist", "FW": "Fascist", "FL": "Fascist", "HL": "Fascist", "LW": "Liberal", "LL": "Liberal"} | |
| return role_to_team.get(self.role) |
| if await _pass_checks(msg_with_poll=msg_with_poll, update=update, context=context): | ||
| await context.bot.stop_poll(update.effective_chat.id, msg_with_poll.id) | ||
|
|
||
| poll_data = context.bot_data[msg_with_poll.poll.id] | ||
| await asyncio.gather( | ||
| *[ | ||
| save_record( | ||
| Record( | ||
| creator_id=poll_data["creator_id"], | ||
| player_id=player_id, | ||
| playroom_id=poll_data["chat_id"], | ||
| game_id=poll_data["message_id"], | ||
| role=result, | ||
| ), | ||
| ) | ||
| for player_id, result in poll_data["results"].items() | ||
| ] | ||
| ) | ||
| records = [ | ||
| Record( | ||
| creator_id=poll_data["creator_id"], | ||
| player_id=player_id, | ||
| playroom_id=poll_data["chat_id"], | ||
| game_id=poll_data["message_id"], | ||
| role=result, | ||
| ) | ||
| for player_id, result in poll_data["results"].items() | ||
| ] | ||
| # await asyncio.gather(*[save_record(record) for record in records]) | ||
| game = Game( | ||
| poll_id=poll_data["message_id"], | ||
| chat_id=poll_data["chat_id"], | ||
| creator_id=poll_data["creator_id"], | ||
| results=poll_data["results"].copy(), | ||
| ) | ||
| await save_game(game) | ||
| await update.effective_message.reply_text( | ||
| "The Game has been saved!. Results: {}".format(game.results) | ||
| ) | ||
| # Delete the poll | ||
| await context.bot.delete_message( | ||
| chat_id=game.chat_id, | ||
| message_id=game.poll_id | ||
| # post-game tasks | ||
| await asyncio.gather( | ||
| *[ | ||
| *[save_record(record) for record in records], | ||
| save_game(game), | ||
| context.bot.delete_message( | ||
| chat_id=game.chat_id, message_id=game.poll_id | ||
| ), | ||
| update.effective_message.delete(), | ||
| context.bot.send_photo( | ||
| photo=( | ||
| await draw_result_image( | ||
| records=records, | ||
| result=game.results, | ||
| update=update, | ||
| context=context, | ||
| ) | ||
| ), | ||
| chat_id=game.chat_id, | ||
| caption="The Game has been saved!", | ||
| disable_notification=True, | ||
| ), | ||
| ] |
There was a problem hiding this comment.
The updated save function includes a comprehensive approach to processing and saving game records, as well as handling additional post-game tasks such as deleting messages and sending a photo with game results. This enhances the game's interactivity and user engagement. However, ensure that error handling is in place for each asynchronous operation within asyncio.gather to gracefully handle any failures.
Consider adding try-except blocks around operations that might fail, such as save_record, save_game, and context.bot.send_photo, to ensure the application remains robust and user-friendly even when unexpected errors occur.
src/services/draw_result_image.py
Outdated
| def create_board(svg, board_type, players): | ||
| if board_type == "Fascist": | ||
| board = SubElement(svg, "g", id="F_BOARD_GROUP") | ||
| x, y, width, height = "56", "518", "1226", "360" | ||
| fill, stroke, stroke_width = FASCIST_COLOR, FASCIST_COLOR_STROKE, STROKE_SIZE | ||
| else: # Liberal | ||
| board = SubElement(svg, "g", id="L_BOARD_GROUP") | ||
| x, y, width, height = "56", "48", "1226", "360" | ||
| fill, stroke, stroke_width = LIBERAL_COLOR, LIBERAL_COLOR_STROKE, STROKE_SIZE | ||
|
|
||
| board_rect = SubElement( | ||
| board, | ||
| "rect", | ||
| id=f"{board_type.upper()}_BOARD", | ||
| x=x, | ||
| y=y, | ||
| width=width, | ||
| height=height, | ||
| rx="6", | ||
| fill=fill, | ||
| stroke=stroke, | ||
| stroke_width=stroke_width, | ||
| ) | ||
|
|
||
| name_rect = SubElement( | ||
| board, | ||
| "rect", | ||
| id=f"{board_type.upper()}_BOARD_NAME", | ||
| x=x, | ||
| y=y, | ||
| width=width, | ||
| height="110", | ||
| fill=fill, | ||
| stroke=stroke, | ||
| stroke_width=str(int(stroke_width) / 2), | ||
| fill_opacity="0.6", | ||
| ) | ||
| board_name_text = SubElement( | ||
| board, | ||
| "text", | ||
| x=str(int(x) + int(width) / 2), | ||
| y=str(int(y) + 55), | ||
| font_size="60", | ||
| fill="black", | ||
| text_anchor="middle", | ||
| dominant_baseline="middle", | ||
| ) | ||
| board_name_text.text = board_type | ||
| player_x = ( | ||
| int(x) + int(width) // 2 - (len(players) * 170 + (len(players) - 1) * 30) // 2 | ||
| ) | ||
| for i, player in enumerate(players): | ||
| # item_group = SubElement(board, "g", id=f"item_{i}") | ||
| # with tempfile.NamedTemporaryFile(suffix=".png", delete=False) as pic_file: | ||
| # pic_file.write(requests.get(player["user_profile_photo"]).content) | ||
| user_pic = SubElement( | ||
| board, | ||
| "image", | ||
| href=player["user_profile_photo"], | ||
| x=str(player_x), | ||
| y=str(int(y) + 117), | ||
| stroke=stroke, | ||
| stroke_width=str(int(stroke_width) // 2), | ||
| height="170", | ||
| width="170", | ||
| ) | ||
| username_rect = SubElement( | ||
| board, | ||
| "rect", | ||
| id=f"username_{i}", | ||
| x=str(player_x), | ||
| y=str(int(y) + 288), | ||
| stroke=stroke, | ||
| stroke_width=str(int(stroke_width) // 2), | ||
| width="170", | ||
| height="55", | ||
| fill="white", | ||
| ) | ||
| username_text = SubElement( | ||
| board, | ||
| "text", | ||
| x=str(player_x + 170 / 2), | ||
| y=str(int(y) + 288 + 55 / 2), | ||
| font_size="20", | ||
| fill="black", | ||
| text_anchor="middle", | ||
| dominant_baseline="middle", | ||
| ) | ||
|
|
||
| username_text.text = player["name"] | ||
| player_x += 200 # 170 width + 30 space | ||
|
|
There was a problem hiding this comment.
The create_board function dynamically generates SVG elements based on the board type and player information. This is a creative way to visualize game boards and player positions. However, the commented-out code related to fetching and writing user profile photos to temporary files (lines 110-112) should either be removed or implemented if needed. Leaving commented-out code can lead to confusion and clutter in the codebase.
- # with tempfile.NamedTemporaryFile(suffix=".png", delete=False) as pic_file:
- # pic_file.write(requests.get(player["user_profile_photo"]).content)Consider removing the commented-out code or implementing the functionality with proper error handling and cleanup of temporary files.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def create_board(svg, board_type, players): | |
| if board_type == "Fascist": | |
| board = SubElement(svg, "g", id="F_BOARD_GROUP") | |
| x, y, width, height = "56", "518", "1226", "360" | |
| fill, stroke, stroke_width = FASCIST_COLOR, FASCIST_COLOR_STROKE, STROKE_SIZE | |
| else: # Liberal | |
| board = SubElement(svg, "g", id="L_BOARD_GROUP") | |
| x, y, width, height = "56", "48", "1226", "360" | |
| fill, stroke, stroke_width = LIBERAL_COLOR, LIBERAL_COLOR_STROKE, STROKE_SIZE | |
| board_rect = SubElement( | |
| board, | |
| "rect", | |
| id=f"{board_type.upper()}_BOARD", | |
| x=x, | |
| y=y, | |
| width=width, | |
| height=height, | |
| rx="6", | |
| fill=fill, | |
| stroke=stroke, | |
| stroke_width=stroke_width, | |
| ) | |
| name_rect = SubElement( | |
| board, | |
| "rect", | |
| id=f"{board_type.upper()}_BOARD_NAME", | |
| x=x, | |
| y=y, | |
| width=width, | |
| height="110", | |
| fill=fill, | |
| stroke=stroke, | |
| stroke_width=str(int(stroke_width) / 2), | |
| fill_opacity="0.6", | |
| ) | |
| board_name_text = SubElement( | |
| board, | |
| "text", | |
| x=str(int(x) + int(width) / 2), | |
| y=str(int(y) + 55), | |
| font_size="60", | |
| fill="black", | |
| text_anchor="middle", | |
| dominant_baseline="middle", | |
| ) | |
| board_name_text.text = board_type | |
| player_x = ( | |
| int(x) + int(width) // 2 - (len(players) * 170 + (len(players) - 1) * 30) // 2 | |
| ) | |
| for i, player in enumerate(players): | |
| # item_group = SubElement(board, "g", id=f"item_{i}") | |
| # with tempfile.NamedTemporaryFile(suffix=".png", delete=False) as pic_file: | |
| # pic_file.write(requests.get(player["user_profile_photo"]).content) | |
| user_pic = SubElement( | |
| board, | |
| "image", | |
| href=player["user_profile_photo"], | |
| x=str(player_x), | |
| y=str(int(y) + 117), | |
| stroke=stroke, | |
| stroke_width=str(int(stroke_width) // 2), | |
| height="170", | |
| width="170", | |
| ) | |
| username_rect = SubElement( | |
| board, | |
| "rect", | |
| id=f"username_{i}", | |
| x=str(player_x), | |
| y=str(int(y) + 288), | |
| stroke=stroke, | |
| stroke_width=str(int(stroke_width) // 2), | |
| width="170", | |
| height="55", | |
| fill="white", | |
| ) | |
| username_text = SubElement( | |
| board, | |
| "text", | |
| x=str(player_x + 170 / 2), | |
| y=str(int(y) + 288 + 55 / 2), | |
| font_size="20", | |
| fill="black", | |
| text_anchor="middle", | |
| dominant_baseline="middle", | |
| ) | |
| username_text.text = player["name"] | |
| player_x += 200 # 170 width + 30 space | |
| def create_board(svg, board_type, players): | |
| if board_type == "Fascist": | |
| board = SubElement(svg, "g", id="F_BOARD_GROUP") | |
| x, y, width, height = "56", "518", "1226", "360" | |
| fill, stroke, stroke_width = FASCIST_COLOR, FASCIST_COLOR_STROKE, STROKE_SIZE | |
| else: # Liberal | |
| board = SubElement(svg, "g", id="L_BOARD_GROUP") | |
| x, y, width, height = "56", "48", "1226", "360" | |
| fill, stroke, stroke_width = LIBERAL_COLOR, LIBERAL_COLOR_STROKE, STROKE_SIZE | |
| board_rect = SubElement( | |
| board, | |
| "rect", | |
| id=f"{board_type.upper()}_BOARD", | |
| x=x, | |
| y=y, | |
| width=width, | |
| height=height, | |
| rx="6", | |
| fill=fill, | |
| stroke=stroke, | |
| stroke_width=stroke_width, | |
| ) | |
| name_rect = SubElement( | |
| board, | |
| "rect", | |
| id=f"{board_type.upper()}_BOARD_NAME", | |
| x=x, | |
| y=y, | |
| width=width, | |
| height="110", | |
| fill=fill, | |
| stroke=stroke, | |
| stroke_width=str(int(stroke_width) / 2), | |
| fill_opacity="0.6", | |
| ) | |
| board_name_text = SubElement( | |
| board, | |
| "text", | |
| x=str(int(x) + int(width) / 2), | |
| y=str(int(y) + 55), | |
| font_size="60", | |
| fill="black", | |
| text_anchor="middle", | |
| dominant_baseline="middle", | |
| ) | |
| board_name_text.text = board_type | |
| player_x = ( | |
| int(x) + int(width) // 2 - (len(players) * 170 + (len(players) - 1) * 30) // 2 | |
| ) | |
| for i, player in enumerate(players): | |
| user_pic = SubElement( | |
| board, | |
| "image", | |
| href=player["user_profile_photo"], | |
| x=str(player_x), | |
| y=str(int(y) + 117), | |
| stroke=stroke, | |
| stroke_width=str(int(stroke_width) // 2), | |
| height="170", | |
| width="170", | |
| ) | |
| username_rect = SubElement( | |
| board, | |
| "rect", | |
| id=f"username_{i}", | |
| x=str(player_x), | |
| y=str(int(y) + 288), | |
| stroke=stroke, | |
| stroke_width=str(int(stroke_width) // 2), | |
| width="170", | |
| height="55", | |
| fill="white", | |
| ) | |
| username_text = SubElement( | |
| board, | |
| "text", | |
| x=str(player_x + 170 / 2), | |
| y=str(int(y) + 288 + 55 / 2), | |
| font_size="20", | |
| fill="black", | |
| text_anchor="middle", | |
| dominant_baseline="middle", | |
| ) | |
| username_text.text = player["name"] | |
| player_x += 200 # 170 width + 30 space |
| async def draw_result_image( | ||
| update: Update, | ||
| context: ContextTypes.DEFAULT_TYPE, | ||
| records: list[Record], | ||
| result: str, | ||
| ) -> bytes: | ||
| """Send the result of the game to the chat""" | ||
| # player = namedtuple("Player", ["name", "role", "team", "user_profile_photo"]) | ||
| players = tuple( | ||
| await asyncio.gather( | ||
| *[get_player(context=context, record=record) for record in records] | ||
| ) | ||
| ) | ||
| svg = draw_game_result(players, result) | ||
| return svg2png(svg) |
There was a problem hiding this comment.
The draw_result_image function orchestrates the generation of the game result image by assembling player information, creating the SVG, and converting it to PNG. This function effectively leverages asynchronous operations to fetch player data concurrently, which can improve performance. Ensure that error handling is robust, especially for external calls like get_player and svg2png, to gracefully handle any issues that may arise during image generation.
Consider adding error handling around asynchronous calls and external operations to ensure the function can recover gracefully from failures and still provide meaningful feedback to the user.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
pyproject.tomlis excluded by:!**/*.toml
Files selected for processing (9)
- src/config.py (1 hunks)
- src/data_models/Game.py (2 hunks)
- src/data_models/Player.py (2 hunks)
- src/data_models/Poll.py (1 hunks)
- src/data_models/Record.py (2 hunks)
- src/handlers/game.py (2 hunks)
- src/handlers/save.py (2 hunks)
- src/services/db_service.py (1 hunks)
- src/services/draw_result_image.py (1 hunks)
Files skipped from review due to trivial changes (2)
- src/config.py
- src/services/db_service.py
Files skipped from review as they are similar to previous changes (5)
- src/data_models/Game.py
- src/data_models/Player.py
- src/data_models/Record.py
- src/handlers/game.py
- src/services/draw_result_image.py
Additional comments: 2
src/data_models/Poll.py (1)
- 1-1: The change to import
Literalfrom thetypingmodule instead ofdatetimeis correct and improves type checking for thepoll_typefield in thePollclass. This is a good practice for specifying exact values that a variable can take.src/handlers/save.py (1)
- 9-9: The import of
draw_result_imageintroduces new functionality for generating game result images. This is a valuable addition to enhance user engagement by providing visual game summaries.
| await asyncio.gather( | ||
| *[ | ||
| *[save_record(record) for record in records], | ||
| save_game(game), | ||
| context.bot.delete_message( | ||
| chat_id=game.chat_id, message_id=game.poll_id | ||
| ), | ||
| update.effective_message.delete(), | ||
| context.bot.send_photo( | ||
| photo=( | ||
| await draw_result_image( | ||
| records=records, | ||
| result=game.results, | ||
| update=update, | ||
| context=context, | ||
| ) | ||
| ), | ||
| chat_id=game.chat_id, | ||
| caption="The Game has been saved!", | ||
| disable_notification=True, | ||
| ), | ||
| ] |
There was a problem hiding this comment.
While the integration of draw_result_image and the restructuring of post-game tasks within asyncio.gather are significant improvements, it's crucial to implement error handling for these asynchronous operations. This ensures the application remains robust and user-friendly, especially when dealing with external services or potential failures in database operations or message sending.
Consider wrapping each operation in try-except blocks or using a more sophisticated error handling strategy for tasks executed with asyncio.gather to manage exceptions gracefully.
Summary by CodeRabbit