Migrate binary_tree module to Conductor#790
Conversation
Rewrites the binary_tree bundle as a Conductor BaseModulePlugin, backed by IDataHandler pair/list primitives instead of js-slang's stdlib. Tree entries are stored as OPAQUE values at the module boundary; is_tree and is_empty_tree declare no arg type so they can accept any DataType and answer false rather than throw, matching their predicate semantics. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![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 migrates the binary_tree bundle from js-slang to @sourceacademy/conductor, refactoring the tree operations, types, and tests to utilize Conductor's IDataHandler and TypedValue system, and exporting the module via a new BinaryTreeModulePlugin class. The review feedback suggests enhancing robustness by adding null/undefined checks for the value parameter in is_tree, is_empty_tree, and assertNonEmptyTree to prevent runtime crashes. Additionally, it is recommended to throw EvaluatorRuntimeError instead of generic Error objects for arity mismatches in the module plugin methods.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
is_tree/is_empty_tree/assertNonEmptyTree now guard against a missing/undefined value instead of crashing with a raw TypeError, and the plugin's arity-check throws now use EvaluatorRuntimeError (already used elsewhere in this module) instead of a generic Error. Also adds the same __bindExportedMethods() workaround repeat/rune use for the unbound-method issue in BaseModulePlugin.initialise() (conductor#41), since this fix isn't merged upstream yet.
|
@coderabbitai, any review comments? |
|
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
WalkthroughThe binary tree module is migrated from synchronous native structures to Conductor ChangesBinary Tree Conductor Migration
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
master's binary_tree gained this validation while conductor-migration was diverging (make_tree(0, 0, null) previously constructed a malformed tree silently instead of throwing). Ports the same check, using EvaluatorTypeError to match this module's existing Conductor error style rather than modules-lib's InvalidParameterTypeError, which doesn't apply here since this module no longer goes through js-slang.
AaravMalani
left a comment
AaravMalani
left a comment
There was a problem hiding this comment.
Yep, LGTM. The only thing left is documentation, but since that's still pending as per #765, I'm approving it in advance
| export function is_empty_tree(value: BinaryTree): value is EmptyBinaryTree { | ||
| return value === null; | ||
| export function is_empty_tree(value: TypedValue<DataType>): value is TypedValue<DataType.EMPTY_LIST> { | ||
| return value?.type === DataType.EMPTY_LIST; |
There was a problem hiding this comment.
Out of curiosity, when would value be nullish?
| // function (which is how every evaluator calls closures) gets `this === undefined` inside the | ||
| // method body. Same workaround as repeat/rune until the upstream fix in conductor lands | ||
| // (source-academy/conductor#41). | ||
| private __bindExportedMethods() { |
| return await make_tree_func(this.evaluator, value, left, right); | ||
| } | ||
|
|
||
| // No declared arg type: is_tree is a predicate that must accept a value of any |
There was a problem hiding this comment.
Hmm, for this I think we should add a DataType.ANY which just accepts all values. Thoughts @Akshay-2007-1 @martin-henz?
2 participants
Description
Fixes #768
Migrates the
binary_treemodule to be Conductor-ready, following the pattern established by therepeatmodule (#698).binary_treehas no tab/visualization component, so this is scoped entirely tosrc/bundles/binary_tree/.functions.tsis rewritten against Conductor'sIDataHandler(pair_make/pair_head/pair_tail) instead of js-slang'sstdlib/list, so tree structure is built from real Conductor Pairs rather than native JS arrays.index.tsis now aBaseModulePluginsubclass with each exported function as a@moduleMethod-decorated closure, mirroringrepeat.DataType.OPAQUEat the module boundary (arbitrary Source/Python values aren't representable as a single concreteDataType).is_tree/is_empty_treedeclare no argument type, since they're predicates that must accept a value of any type and answerfalserather than throw — a declared type would mean the FFI boundary type-asserts and throws before the predicate body ever runs.tsconfig.jsonpicked up the sameexperimentalDecorators/emitDecoratorMetadataoverridesrepeatalready has (this bundle was missing them, which breaks themoduleMethoddecorator under TS's native Stage-3 decorators).Testing
yarn workspace @sourceacademy/bundle-binary_tree run tsc— passesyarn workspace @sourceacademy/bundle-binary_tree run lint— passesyarn workspace @sourceacademy/bundle-binary_tree run test— 16/16 passing, against@sourceacademy/modules-testplugin'sTestDataHandleryarn workspace @sourceacademy/bundle-binary_tree run build— producesbuild/bundles/binary_tree.jsPyCseEvaluator(via py-slang#217's module loader work) and drovemake_tree/entry/left_branch/right_branch/is_treefrom real Python-shaped values through the actualpythonToModule/moduleToPythonconversion code — all round-trip correctly.Notes for reviewers
Getting the end-to-end test working surfaced two bugs upstream of this PR, not fixable within
binary_treeitself:BaseModulePlugin.initialise()inconductorregisters each exported method as a detached function reference (this[name]) without binding it to the instance, sothisisundefinedinside any exported method body once a real evaluator calls it. This breaks every Conductor module that uses ordinary class methods (i.e. all of them), not just this one. Filing a fix againstconductorseparately.moduleToPython/pythonToModulein py-slang#217 flatten a returnedDataType.PAIRinto a Python list and convert Python lists back toDataType.ARRAY, which loses identity for any function returning a Pair that's meant to be passed back into another module call (e.g.left_branch(t)). Flagging this on that PR directly since the affected file is new there.