Add Windows Compatibility and forward compatibility with Quart #20
Conversation
Reviewer's Guide by SourceryThis pull request addresses compatibility issues with newer Quart versions and fixes test failures on Windows. It modifies the project dependencies to support different Quart versions based on the Python version and conditionally registers a function to close connections before forking on POSIX systems. Updated class diagram for pyproject.toml dependenciesclassDiagram
class pyproject.toml {
dependencies: List[str]
}
note for pyproject.toml "quart dependency changed to support different versions based on Python version"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @Zwork101, 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!
Summary of Changes
This pull request addresses compatibility issues with the quart-sqlalchemy library. Specifically, it ensures compatibility with newer versions of Quart (>=0.20.0) while maintaining backward compatibility for Python 3.8. It also fixes a test failure on Windows by conditionally registering the close_connections_for_forking function only on POSIX systems, as os.fork is not available on Windows.
Highlights
- Quart Compatibility: The pyproject.toml file is modified to allow installation of newer versions of Quart (>=0.20.0) for Python versions >= 3.9, while maintaining compatibility with older versions for Python < 3.9.
- Windows Compatibility: The
os.register_at_forkcall is now conditionally executed only on POSIX systems to prevent errors on Windows. - Version Update: The version of the package is bumped from 3.0.4 to 3.0.5 in both
pyproject.tomlandsrc/quart_sqlalchemy/__init__.py.
Changelog
- pyproject.toml
- Updated package version from 3.0.4 to 3.0.5.
- Modified dependencies to allow quart>=0.18.3 for python versions >= 3.9 and quart<0.20.0,>=0.18.3 for python versions < 3.9.
- src/quart_sqlalchemy/init.py
- Updated package version from 3.0.4 to 3.0.5.
- src/quart_sqlalchemy/bind.py
- Wrapped the
os.register_at_forkcall in a conditional statement to only execute on POSIX systems.
- Wrapped the
- tox.ini
- Added
allowlist_externalstotestenvandtestenv:q183to allow pdm to run.
- Added
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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.
A fork in the road,
Windows we must now decode,
POSIX guides the way.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request addresses compatibility issues with Quart 0.20.0 and Windows, which is great. The changes look good overall, but I have a few suggestions to improve the code and ensure its robustness.
Summary of Findings
- Conditional import: The use of conditional import based on python version in
pyproject.tomlis a good approach to maintain compatibility with older versions while leveraging newer features. However, ensure that all dependencies are correctly handled for each Python version. - OS check for
register_at_fork: The addition ofif os.name == 'posix'beforeos.register_at_forkis necessary for Windows compatibility. However, consider whether there might be a more robust solution to handle forking in a cross-platform manner. - Version update: The version update in
pyproject.tomland__init__.pyshould be consistent and follow semantic versioning principles.
Merge Readiness
The pull request is almost ready for merging. The changes address important compatibility issues. However, I recommend addressing the comments regarding the version update and the potential for a more robust solution for handling forking. I am unable to approve the pull request, and users should have others review and approve this code before merging. Once these points are addressed, the pull request should be good to go.
| [project] | ||
| name = "quart-sqlalchemy" | ||
| version = "3.0.4" | ||
| version = "3.0.5" |
There was a problem hiding this comment.
It's good to see the version number updated. However, ensure that the versioning scheme follows semantic versioning principles. Consider if a minor or patch version bump is more appropriate based on the changes made.
version = "3.1.0"
| if os.name == 'posix': | ||
| os.register_at_fork(before=close_connections_for_forking) |
There was a problem hiding this comment.
This check for os.name == 'posix' is good for preventing errors on Windows. However, consider if there's a more robust cross-platform solution for handling forking, or if a warning should be logged when running on Windows to indicate that forking-related connection safety measures are not active.
| if os.name == 'posix': | |
| os.register_at_fork(before=close_connections_for_forking) | |
| if os.name == 'posix': | |
| os.register_at_fork(before=close_connections_for_forking) | |
| else: | |
| import logging | |
| logging.warning("os.register_at_fork is not available on Windows. Connection safety during forking is not guaranteed.") |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Zwork101 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using
sys.platforminstead ofos.namefor checking the operating system insrc/quart_sqlalchemy/bind.py. - It might be better to use
packaging.versionto handle version comparisons inpyproject.toml.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
1 participant
I was working on a Quart project when I noticed quart-sqlalchemy lacked compatibility with Quart 0.20.0. I suspect the reason for that is to maintain backwards compatibility with Python 3.8. However, this can be accomplished by modifying the pyproject.toml to install the correct quart package based on python version.
Also, the tests were failing on windows, the culprit being
Which isn't implemented on Windows, since os.fork doesn't work in Windows. Even if it didn't raise an error, it wouldn't do anything. I am a bit concerned that this simple fix is leaving the underlying issue unaddressed, however all tests pass now. For that reason I feel good about submitting this PR, at very least to start the conversation.
Summary by Sourcery
Improve Windows compatibility and Quart version support for quart-sqlalchemy
New Features:
Bug Fixes:
Enhancements:
Chores: