feat: pydump support Python defined class #5
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 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
|
![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
This pull request adds support for parsing Python classes in pydump, which is a great feature enhancement. The changes include new data structures in symbol/symbol.go to represent classes, and the core parsing logic in a new file _xtool/pydump/class.go. The modifications in _xtool/pydump/pydump.go integrate the new class parsing functionality.
My review focuses on ensuring robustness and consistency. I've found a critical issue that could lead to a panic when parsing base classes, and a high-severity issue regarding inconsistent handling of property getters and setters. I've also included some suggestions for code quality and performance improvements. Overall, the changes are well-structured, and with these fixes, the new feature will be a solid addition.
| Name: c.GoString(baseObj.GetAttrString(c.Str("__name__")).CStr()), | ||
| Module: c.GoString(baseObj.GetAttrString(c.Str("__module__")).CStr()), |
There was a problem hiding this comment.
There's a potential for a panic on these lines. If baseObj.GetAttrString returns nil (e.g., if a base class object lacks a __name__ or __module__ attribute), the subsequent call to .CStr() will cause a panic. It's crucial to check for nil before dereferencing.
For example:
nameObj := baseObj.GetAttrString(c.Str("__name__"))
if nameObj == nil {
return nil, fmt.Errorf("a base class of %s has no __name__", name)
}
// ... and then use nameObj| getter := val.GetAttrString(c.Str("fget")) | ||
| if getter != nil { | ||
| // (self) -> value | ||
| property.Getter = "()" |
There was a problem hiding this comment.
The property getter's signature is hardcoded to "()", which loses information about its return type. This is inconsistent with how the setter's signature is parsed using getSignature later in this function. You should consider using getSignature for the getter as well to provide a more complete and accurate signature. This would likely require adding error handling similar to what's done for the setter.
| if basesObj == nil { | ||
| return nil, fmt.Errorf("can't get __bases__ from %s", name) | ||
| } | ||
| bases := make([]*symbol.Base, 0) |
There was a problem hiding this comment.
For a minor performance improvement, you can pre-allocate the bases slice with the required capacity, since the size is known from basesObj.TupleLen(). This avoids potential reallocations during the loop.
| bases := make([]*symbol.Base, 0) | |
| bases := make([]*symbol.Base, 0, basesObj.TupleLen()) |
| case "classmethod": | ||
| val = val.GetAttrString(c.Str("__func__")) | ||
| if val == nil { | ||
| return nil, fmt.Errorf("can't get __func__ of %s", name) | ||
| } | ||
| sym, err := parseMethod(val, name, typeName, true) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| cls.ClassMethods = append(cls.ClassMethods, sym) | ||
| case "staticmethod": | ||
| val = val.GetAttrString(c.Str("__func__")) | ||
| if val == nil { | ||
| return nil, fmt.Errorf("can't get __func__ of %s", name) | ||
| } | ||
| sym, err := parseMethod(val, name, typeName, false) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| cls.StaticMethods = append(cls.StaticMethods, sym) |
There was a problem hiding this comment.
The logic for handling classmethod and staticmethod is very similar and contains duplicated code. You could refactor this to reduce duplication, for example by handling them in the same case block and using a boolean to decide whether to skip the first parameter in the signature.
1 participant
No description provided.