Binding DB sessions based on SQLAlchemy 1, changing how to declare Base Model classes, and other code modernization #4
Conversation
…se Model classes, and other code modernization - DB session binding based on SQLAlchemy 2, Base Model class declaration method change - Reflected select, delete code based on SQLAlchemy 2 - Changed how to declare Model class based on SQLAlchemy 2 - Fixed issue with social connection termination - Added pyproject.toml file after removing setup.py due to the introduction of PEP 517/518 - Fixed minimum installed version to Python 3.7
|
Warning Rate Limit Exceeded@pre-commit-ci[bot] has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 6 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThis update focuses on ensuring compatibility with SQLAlchemy version 2 across various Python social-auth projects, reflecting updates in packaging standards according to PEP 517/518, and fixing specific function errors. It streamlines the build and publish process for developers and addresses necessary adaptations for SQLAlchemy's major version change. Changes
Related issues
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: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
pyproject.tomlis excluded by:!**/*.toml
Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- Makefile (1 hunks)
- social_webpy/app.py (1 hunks)
- social_webpy/models.py (3 hunks)
Additional comments: 8
Makefile (3)
- 2-2: The update to use
python -m buildfor thebuildtarget aligns with modern Python packaging practices and PEP 517/518 standards. This is a positive change that simplifies the build process.- 5-5: Using
twine upload dist/*for thepublishtarget is a secure and recommended way to upload packages to PyPI. This change is in line with best practices for Python package distribution.- 1-8: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [7-10]
The
cleantarget is comprehensive, ensuring a clean state by removing bytecode files,__pycache__directories, and build artifacts (*.egg-info,dist,build). This is crucial for maintaining a clean environment, especially with the updated build process.CHANGELOG.md (1)
- 10-14: The updates to the CHANGELOG.md accurately document the significant changes made in this PR, including adapting to SQLAlchemy 2, updating packaging information, setting the minimum Python version, and fixing an error. These entries are clear, adhere to the Keep a Changelog format, and provide users with important information about the changes.
social_webpy/models.py (3)
- 4-5: The updated imports from SQLAlchemy and the use of
DeclarativeBase,Mapped,mapped_column, andrelationshipare in line with SQLAlchemy 2's methodologies. These changes modernize the ORM models and ensure compatibility with the latest version of SQLAlchemy.- 17-18: The introduction of the
SocialBaseclass as a base class usingDeclarativeBaseis a good practice for defining ORM models in SQLAlchemy 2, providing a clear and consistent base for model classes.- 32-35: The refactoring of the
UserSocialAuthclass to useMapped,mapped_column, andrelationshipaligns with SQLAlchemy 2's ORM improvements. This ensures that the model declarations are up-to-date and leverage the latest features and best practices of SQLAlchemy.social_webpy/app.py (1)
- 20-25: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [10-10]
The removal of the
_userattribute assignment in the__init__method ofBaseViewClasssuggests a change in how the current user's state is managed. Please ensure that this change has been thoroughly tested, especially in relation to theget_current_usermethod and any other functionality that relies on the current user's state.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
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 (3)
- .pre-commit-config.yaml (1 hunks)
- social_webpy/app.py (2 hunks)
- social_webpy/models.py (3 hunks)
Files skipped from review due to trivial changes (1)
- .pre-commit-config.yaml
Additional comments: 6
social_webpy/models.py (4)
- 16-17: The
SocialBaseclass is correctly defined as a placeholder for further extensions. However, ensure thatSocialBaseis instantiated fromdeclarative_base()as suggested in the previous comment.- 32-35: The
UserSocialAuthmodel class correctly usesmapped_columnandMappedtypes, aligning with SQLAlchemy 2 standards. Ensure thatUser.idis correctly defined in the user model and that the relationship'sbackrefis properly set up.- 1-8: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [42-60]
The model classes
Nonce,Association,Code, andPartialare correctly defined as placeholders, inheriting behavior from their respective mixins andSocialBase. No specific fields or methods are added, which is appropriate for their intended use.
- 1-8: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [62-68]
The
WebpyStorageclass correctly aggregates the model classes for use in the storage backend, ensuring that each component of the social authentication process is properly managed.social_webpy/app.py (2)
- 5-5: The import statement has been updated to remove duplicates, which is a good practice for maintaining clean and readable code. Ensure that all necessary utilities are still imported for the application's functionality.
- 20-25: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [11-29]
The
BaseViewClassconstructor and methods appear to be correctly structured, with appropriate session and strategy handling. However, the AI-generated summary mentioned changes related to the_userattribute that are not visible in the provided code. Ensure that any modifications to the_userattribute handling are correctly implemented and tested.
social_webpy/models.py
Outdated
| from sqlalchemy import String, ForeignKey | ||
| from sqlalchemy.orm import relationship, DeclarativeBase, Mapped, mapped_column |
There was a problem hiding this comment.
The import statements have been updated to align with SQLAlchemy 2 standards. However, it's important to ensure that DeclarativeBase is correctly imported from sqlalchemy.orm as declarative_base to maintain consistency with SQLAlchemy's expected usage patterns.
- from sqlalchemy.orm import relationship, DeclarativeBase, Mapped, mapped_column
+ from sqlalchemy.orm import relationship, declarative_base, Mapped, mapped_column
+ SocialBase = declarative_base()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.
| from sqlalchemy import String, ForeignKey | |
| from sqlalchemy.orm import relationship, DeclarativeBase, Mapped, mapped_column | |
| from sqlalchemy import String, ForeignKey | |
| from sqlalchemy.orm import relationship, declarative_base, Mapped, mapped_column | |
| SocialBase = declarative_base() |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (6)
- .travis.yml (1 hunks)
- social_webpy/init.py (1 hunks)
- social_webpy/app.py (3 hunks)
- social_webpy/models.py (2 hunks)
- social_webpy/strategy.py (4 hunks)
- social_webpy/utils.py (2 hunks)
Files skipped from review due to trivial changes (3)
- social_webpy/init.py
- social_webpy/strategy.py
- social_webpy/utils.py
Files skipped from review as they are similar to previous changes (2)
- social_webpy/app.py
- social_webpy/models.py
| python: | ||
| - "2.7" | ||
| - '2.7' |
There was a problem hiding this comment.
The Python version 2.7 is specified, but the PR objectives mention setting the minimum Python version to 3.7. This version should be updated or removed to align with the new minimum version requirement.
- - '2.7'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.
| - '2.7' |
| - python: pypy | ||
| env: | ||
| - TEST_REQUIREMENTS=social/tests/requirements-pypy.txt | ||
| - python: '3.3' |
There was a problem hiding this comment.
Python version 3.3 is specified, which is below the new minimum Python version of 3.7. This version should be updated or removed to reflect the updated requirements.
- - python: '3.3'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.
| - python: '3.3' |
| env: | ||
| - REQUIREMENTS=requirements-python3.txt | ||
| - TEST_REQUIREMENTS=social/tests/requirements-python3.txt | ||
| - python: '3.4' |
There was a problem hiding this comment.
Python version 3.4 is specified, which is also below the new minimum Python version of 3.7. This version should be updated or removed to comply with the new version requirements.
- - python: '3.4'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.
| - python: '3.4' |
| - python setup.py -q install | ||
| - travis_retry pip install -r $REQUIREMENTS | ||
| - travis_retry pip install -r $TEST_REQUIREMENTS |
There was a problem hiding this comment.
The installation commands use python setup.py -q install, which might not align with the transition to PEP 517/518 as mentioned in the PR objectives. Consider using a more modern approach for package installation that is compatible with PEP 517/518, such as using pip to install the package directly from the source with the pyproject.toml file.
- - python setup.py -q install
+ - pip install .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.
| - python setup.py -q install | |
| - travis_retry pip install -r $REQUIREMENTS | |
| - travis_retry pip install -r $TEST_REQUIREMENTS | |
| - pip install . | |
| - travis_retry pip install -r $REQUIREMENTS | |
| - travis_retry pip install -r $TEST_REQUIREMENTS |
|
Thanks for your contribution. This project really lacks maintenance as current maintainers are only interested in the core and Django parts, but I've decided it's better to merge this now. |
Proposed changes
With the introduction of PEP 517/518, the Python package build process has changed to allow the use of multiple build backends. We've improved the build process accordingly.
In addition, we improved the program to ensure that several previously developed libraries work properly with the newly updated SQLAlchemy 2 in January 2023.
Please consider this PR and look forward to the new version release. I took the time to fix the program and submit the patch because I'm planning to include it in a Korean Flask-based programming book I'm writing.
Types of changes
Please check the type of change your PR introduces:
Checklist
Put an
xin the boxes that apply. You can also fill these out after creatingthe PR. If you're unsure about any of them, don't hesitate to ask. We're here to
help! This is simply a reminder of what we are going to look for before merging
your code.
Summary by CodeRabbit