feat(graph): add Python code graph extractor#103
Conversation
theDakshJaitly
left a comment
theDakshJaitly
left a comment
There was a problem hiding this comment.
Thanks for taking on the first community language extractor. The implementation is cleanly registered and the current suite/typecheck/build pass, but a few #94 acceptance criteria are not met yet—most importantly instantiation references and bindable cross-file Python imports. I also found two AST-walk correctness issues below. Please address the inline comments, document the vendored grammar's source/version/license, and remove or split out the unrelated setup.sh / Windows CLI-test changes.
| calleeName = getNodeText(fn, this.source); | ||
| } | ||
| } | ||
| // We treat all calls as 'calls', since Python has no 'new' keyword to distinguish 'instantiates'. |
There was a problem hiding this comment.
Blocking: #94 explicitly requires instantiates references, but this path emits every constructor-shaped call as calls. That also prevents Car(...) from ever binding, because the resolver only allows calls to target functions/methods while instantiates can target classes. Please emit and test an instantiates reference for the supported constructor heuristic (and preserve calls only where appropriate).
| } | ||
| } | ||
| } else if (node.type === "import_from_statement") { | ||
| const moduleNameNode = getChildByField(node, "module_name") ?? node.namedChild(0); |
There was a problem hiding this comment.
Blocking: these Python module names are not currently bindable by the existing resolution pass. The resolver only tries JS/TS extensions and JS-style relative paths; additionally, from . import local becomes only targetName: ".", losing local. Please add an end-to-end two-file Python fixture/test and preserve a specifier the resolver can bind, including the necessary Python resolution support.
| private extractHeritage(node: TSNode, fromId: string): void { | ||
| const superclasses = getChildByField(node, "superclasses"); | ||
| if (!superclasses) return; | ||
| for (const child of superclasses.namedChildren) { |
There was a problem hiding this comment.
Please filter the superclass argument list to actual base expressions. As written, class Child(Base, metaclass=type) emits both extends Base and the invalid extends metaclass=type. A focused test covering a metaclass keyword would protect this.
| // We treat all calls as 'calls', since Python has no 'new' keyword to distinguish 'instantiates'. | ||
| if (calleeName) this.addRef(ownerId, calleeName, "calls", node); | ||
|
|
||
| const args = getChildByField(node, "arguments"); |
There was a problem hiding this comment.
Nested argument calls are walked twice: once here and again when walkBody continues through all of the call node's children. For outer(inner()), the extractor emits two identical inner references. Please make one traversal path responsible for arguments and add a regression assertion.
| tsx: "tree-sitter-tsx.wasm", | ||
| javascript: "tree-sitter-javascript.wasm", | ||
| jsx: "tree-sitter-javascript.wasm", | ||
| python: "tree-sitter-python.wasm", |
There was a problem hiding this comment.
The vendored Python WASM needs provenance documentation before merge. #94 and docs/extractors.md require the grammar source, exact version/commit, and license; none is included in this diff. Please add that documentation alongside the registration.
2 participants
What
LanguageExtractorfor the code graph..pyfile detection.targetName.Why
Closes #94.
This adds first-class Python language support to the code-graph preview branch while preserving the frozen
LanguageExtractorinterface and existing graph semantics.Type of change
How to test
npm test.py) are detected correctly.targetName.Checklist
npm test)Code-graph preview
code-graph-preview, notmainTarget branch: code-graph-previewLanguageExtractororFrameworkResolverinterfacecore / discuss-firstissue is linked above