feat: add image aspect ratio setting #184
Conversation
Summary of ChangesHello @DankerMu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the application's settings by introducing a user-configurable image aspect ratio. This feature provides greater control over image generation, supported by robust backend logic for validation and normalization. The changes also include an update to the HTTP client used in integration tests, moving from Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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
The pull request introduces an image aspect ratio setting, including UI integration, server-side validation, and normalization. The backend logic correctly handles various aspect ratio inputs, reducing them to their simplest form and validating against a predefined range and common ratios. Integration tests have been updated to use httpx instead of requests, and new unit tests cover the aspect ratio functionality. However, there's an unnecessary dependency on requests added to pyproject.toml and uv.lock, and an arbitrary length check for normalized aspect ratios that could lead to unexpected rejections.
| ) | ||
|
|
||
| normalized = f"{width}:{height}" | ||
| if len(normalized) > 10: |
There was a problem hiding this comment.
The check len(normalized) > 10 seems arbitrary and could lead to valid, simplified aspect ratios being rejected. For example, a ratio like 10000:1 would simplify to 10000:1 (length 7) but 100000:1 would simplify to 100000:1 (length 8). If a user inputs 1000000000:1, it would simplify to 1000000000:1 (length 10), which is fine. However, if a ratio like 1234567890:1 is simplified to 1234567890:1 (length 11), it would be rejected. Given that custom ratios are allowed, this length check might be too restrictive and not directly related to the validity or extremity of the ratio itself. Consider removing this check or basing it on a more logical constraint if there's a specific reason for it.
| "python-pptx>=1.0.0", | ||
| "python-dotenv>=1.0.1", | ||
| "reportlab>=4.1.0", | ||
| "requests>=2.0.0", |
There was a problem hiding this comment.
The integration tests (backend/tests/integration/test_api_full_flow.py) have been updated to use httpx instead of requests. If requests is no longer used anywhere else in the backend, it should be removed from the dependencies list to keep the project dependencies lean and avoid unnecessary packages.
| { name = "python-dotenv" }, | ||
| { name = "python-pptx" }, | ||
| { name = "reportlab" }, | ||
| { name = "requests" }, |
There was a problem hiding this comment.
Similar to the pyproject.toml comment, if requests is no longer a direct dependency after migrating to httpx in the integration tests, this entry should be removed from uv.lock to reflect the actual project dependencies.
| { name = "python-dotenv", specifier = ">=1.0.1" }, | ||
| { name = "python-pptx", specifier = ">=1.0.0" }, | ||
| { name = "reportlab", specifier = ">=4.1.0" }, | ||
| { name = "requests", specifier = ">=2.0.0" }, |
There was a problem hiding this comment.
This requires-dist entry for requests should also be removed if the library is no longer a dependency, aligning with the removal from pyproject.toml.
|
Review summary (local)
LGTM (waiting for CI). |
1 participant
Summary
Adds an image aspect ratio setting to the Settings UI and validates it server-side.
Changes
image_aspect_ratioselectorTesting
Closes #167