-
Notifications
You must be signed in to change notification settings - Fork 3
feat: support global variables #39
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @toaction, 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 significantly enhances the system's capability by introducing comprehensive support for Python global variables. It extends the 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
|
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.
Code Review
This pull request introduces support for generating Go bindings for global variables from Python modules. The changes are well-structured, adding logic to pydump to identify variable types, updating the symbol.Module struct, and implementing the code generation in a new genvar.go file. The new functionality is also covered by a test case, which is great. I have one suggestion regarding the performance of the name conflict resolution logic, which could be improved for larger modules.
Code Review SummaryExcellent implementation of global variable support! 🎯 The feature successfully extends existing patterns and maintains codebase consistency. Critical Issues to Address:
Strengths:
Please address the inline comments before merging. The security and performance issues are particularly important for production use. |
|
need review @luoliwoshang |
_xtool/pydump/pydump.go
Outdated
| var pyVarTypes = map[string]struct{}{ | ||
| "int": {}, | ||
| "float": {}, | ||
| "complex": {}, | ||
| "bool": {}, | ||
| "str": {}, | ||
| "list": {}, | ||
| "tuple": {}, | ||
| "set": {}, | ||
| "dict": {}, | ||
| "NoneType": {}, | ||
| } |
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 name pyVarTypes is misleading - it suggests "variable types" but actually stores Python data type names. Variables don't have types, their values do. Are all Python basic types included here? It seems to be missing
bytes, frozenset, and range types.
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.
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.
When you mention "those that can be directly declared", are you referring to global variables that can be created using literal syntax?
For constructor-declared global variables, they are still global variables. Is this not intended to be supported, or is there currently no way to support it? If it's temporarily unsupported but we plan to support it in the future, this should be clearly documented in the comments.
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.
Due to hardcoding issues, currently we determine whether to generate corresponding Go binding code by checking if the variable's corresponding data type is a basic data type:
// variables
if _, ok := pyBasicDataTypes[sym.Type]; ok {
modInstance.Variables = append(modInstance.Variables, sym)
continue
}After resolving the hardcoding issue, we will support more types of global variables, such as class variables:
class Animal:
pass
DOG = Animal()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 current whitelisting approach only supports a small subset of global variables. Perhaps we could find alternative ways to support this feature! The current architecture seems a bit too ad-hoc.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #39 +/- ##
==========================================
+ Coverage 44.46% 47.05% +2.58%
==========================================
Files 6 7 +1
Lines 452 476 +24
==========================================
+ Hits 201 224 +23
- Misses 232 233 +1
Partials 19 19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1579433 to
ab41361
Compare
fix #22