feat: implement Google Drive uploader and image processing pipeline f…#1
feat: implement Google Drive uploader and image processing pipeline f…#1
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive pipeline for generating conference speaker cards, including data fetching from Sessionize, image processing with background removal, and automated uploading to Google Drive. The feedback focuses on finalizing placeholder layout constants and improving code robustness. Key suggestions include adding timeouts to all network requests, replacing broad exception handling with specific types, and migrating from print statements to a consistent logging framework. Additionally, several PEP 8 violations regarding top-level imports were identified, along with recommendations for better dependency configuration and security practices in example environment files.
| # Layout — pixel coordinates relative to template canvas | ||
| SPEAKER_TARGET_HEIGHT: int = 500 # normalized height of each speaker cutout | ||
| SPEAKER_ANCHOR_Y: int = 820 # PLACEHOLDER: y-pixel where speaker bottom aligns | ||
| SPEAKER_SINGLE_X: int = 270 # PLACEHOLDER: x for single-speaker card | ||
| SPEAKER_LEFT_X: int = 150 # PLACEHOLDER: x for left speaker on dual card | ||
| SPEAKER_RIGHT_X: int = 555 # PLACEHOLDER: x for right speaker on dual card | ||
| TEXT_AREA_Y_START: int = 870 # PLACEHOLDER: y where text block begins | ||
| FONT_SIZE_NAME: int = 48 # PLACEHOLDER | ||
| FONT_SIZE_TITLE: int = 36 # PLACEHOLDER | ||
| TEXT_SHADOW_COLOR: tuple[int, int, int] = (10, 20, 60) | ||
| TEXT_PRIMARY_COLOR: tuple[int, int, int] = (255, 255, 255) | ||
| TEXT_SAFE_WIDTH_RATIO: float = 0.85 |
There was a problem hiding this comment.
This module defines several layout constants with comments indicating they are PLACEHOLDER values. The generated images will not be correct until these placeholders are replaced with the final, accurate coordinates and sizes. Merging this in its current state will result in a non-functional feature.
| @@ -0,0 +1,3 @@ | |||
| SESSIONIZE_API_SLUG=prcjw6ue | |||
There was a problem hiding this comment.
The SESSIONIZE_API_SLUG appears to contain a real value. To prevent accidental leaks of production or staging identifiers, it's best practice to use a generic placeholder in example files, for example your-sessionize-slug.
SESSIONIZE_API_SLUG=your-sessionize-slug
| @@ -0,0 +1,73 @@ | |||
| import logging | |||
| import os | |||
There was a problem hiding this comment.
The os module is imported but never used. It should be removed to keep the code clean.
|
|
||
| def remove_background(image_bytes: bytes, session: any) -> Image.Image: | ||
| output_bytes = remove(image_bytes, session=session) | ||
| from io import BytesIO |
There was a problem hiding this comment.
Imports should be at the top of the file, as per PEP 8 guidelines. Please move from io import BytesIO to the top-level imports.
|
|
||
| def fetch_session_cards(api_slug: str) -> list[SessionCard]: | ||
| url = f"{SESSIONIZE_BASE_URL}/{api_slug}/view/All" | ||
| response = requests.get(url) |
There was a problem hiding this comment.
It's a good practice to include a timeout for all external network requests. This prevents the application from hanging indefinitely if the remote server is unresponsive.
| response = requests.get(url) | |
| response = requests.get(url, timeout=30) |
| except Exception as e: | ||
| print(f"Error processing speaker {speaker.full_name}: {e}") | ||
| # Continue with other speakers or skip |
There was a problem hiding this comment.
Catching a broad Exception can hide unexpected errors. It's better to catch more specific exceptions, such as requests.exceptions.RequestException. Also, consider using the logging module instead of print for consistency.
| except Exception: | ||
| # Fallback to default font if custom font is missing | ||
| font_name = ImageFont.load_default() | ||
| font_title = ImageFont.load_default() |
There was a problem hiding this comment.
This except block catches any exception silently and falls back to a default font. While the fallback is good, you should catch a more specific exception (like OSError for a missing font file) and log a warning. This would alert developers that the intended font is missing without crashing the program.
| except Exception: | |
| # Fallback to default font if custom font is missing | |
| font_name = ImageFont.load_default() | |
| font_title = ImageFont.load_default() | |
| except OSError: | |
| # Fallback to default font if custom font is missing | |
| # It's recommended to log a warning here. | |
| font_name = ImageFont.load_default() | |
| font_title = ImageFont.load_default() |
| with patch("src.gdrive_uploader.service_account.Credentials.from_service_account_file") as mock_creds, \ | ||
| patch("src.gdrive_uploader.build") as mock_build, \ |
There was a problem hiding this comment.
The backslash \ for line continuation is redundant inside parentheses. It can be removed for cleaner code.
with patch("src.gdrive_uploader.service_account.Credentials.from_service_account_file") as mock_creds,
patch("src.gdrive_uploader.build") as mock_build,| import unittest.mock as mock | ||
|
|
||
| import src.generate_cards |
There was a problem hiding this comment.
Imports should be at the top of the file, as per PEP 8 guidelines. Please move these imports to the top of the file. This applies to other test functions in this file as well.
| # Since we can't easily mock font metrics, we'll check the logic with a small max_width | ||
| text = "This is a long title that should wrap several times" | ||
| # Mock font (default font roughly 6px wide per char) | ||
| from PIL import ImageFont |
There was a problem hiding this comment.
Imports should be at the top of the file, as per PEP 8 guidelines. Please move this import to the top of the file. This applies to other local imports in this test file as well (e.g., unittest.mock, src.image_processor).
1 participant
…or session cards