-
Notifications
You must be signed in to change notification settings - Fork 92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make the import hook compatible with python3.12 #230
Conversation
b9a67bc
to
9e537ed
Compare
@coderabbitai review |
Warning Rate Limit Exceeded@aisk has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 9 minutes and 1 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. 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. WalkthroughThe changes involve updating the import hook mechanism in the Changes
Assessment against linked issues
Poem
TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- .gitignore (1 hunks)
- thriftpy2/hook.py (1 hunks)
Files skipped from review due to trivial changes (1)
- .gitignore
Additional comments: 1
thriftpy2/hook.py (1)
- 14-16: The comment indicates a known issue with the load process not being compatible with the Python standard. It would be beneficial to address this issue or provide more context on how it might be resolved in the future.
thriftpy2/hook.py
Outdated
def exec_module(self, module): | ||
... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exec_module
method in the ThriftLoader
class is not implemented and contains only an ellipsis. This method should be properly implemented to execute the module's code within the given module's namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need update the module, the module's content is already modified by create_module
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aisk Understood. If the module's content is being adequately handled by create_module
, then leaving exec_module
as is makes sense according to your implementation strategy. Thank you for the clarification.
Also a little refactor on the old
ThriftImporter
, since we can using theis
operator to compare to class instances, so the__eq__
overload is not needed.fix: #229
Summary by CodeRabbit
New Features
Refactor
Chores
.gitignore
to include the 'venv' directory, indicating a virtual environment setup.