-
Notifications
You must be signed in to change notification settings - Fork 0
Add term.match() to Python and TypeScript bindings #93
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
Conversation
Co-authored-by: hzhangxyz <11623447+hzhangxyz@users.noreply.github.com>
Co-authored-by: hzhangxyz <11623447+hzhangxyz@users.noreply.github.com>
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.
Pull request overview
This PR adds the missing term_match function to both Python and TypeScript bindings, enabling term unification with scope support. The implementation follows the existing patterns in the codebase for similar binding functions like term_ground and term_rename, with appropriate handling of scope parameters (passing empty strings rather than nullptr, as the match operation uses scopes to label variables in the result dictionary).
Key changes:
- Added C++ binding functions (
term_match) in both Python and TypeScript binding files - Implemented high-level
match()methods in PythonTermclass and TypeScriptterm_tclass - Added comprehensive test coverage with 4 test cases each (simple, complex, fail, and scoped scenarios)
- Updated API documentation for both languages
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
apyds/ds.cc |
Added term_match C++ binding function and registered it in the Python module |
apyds/term_t.py |
Implemented match() method in the Term class with proper None-to-empty-string conversion |
atsds/ds.cc |
Added term_match C++ binding function and registered it in the TypeScript module |
atsds/index.mts |
Implemented match() method in the term_t class with default empty string parameters |
tests/test_term.py |
Added 4 comprehensive test cases covering various match scenarios |
tests/test_term.mjs |
Added 4 comprehensive test cases mirroring the Python tests |
docs/api/python.md |
Added API documentation for the match() method |
docs/api/typescript.md |
Added API documentation for the match() method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
docs/api/typescript.md
Outdated
| const b = new term_t("b"); | ||
| const result = a.match(b); | ||
| if (result !== null) { | ||
| console.log(result.toString()); // "(( `a b))" |
Copilot
AI
Dec 10, 2025
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 expected output in the comment is incorrect. When scope parameters are not provided (or are empty strings), the result should include two spaces for the empty scopes. The output should be "(( \a b))"(with two spaces after the opening parentheses) instead of"((`a b))"`.
This matches the test expectation in tests/test_term.mjs line 109 and is consistent with how the underlying C++ implementation handles empty scopes.
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.
Fixed in f9114ab. Updated to show "((r f `a b))" with hardcoded scopes "r" and "f".
atsds/index.mts
Outdated
| * const b = new term_t("b"); | ||
| * const result = a.match(b); | ||
| * if (result !== null) { | ||
| * console.log(result.toString()); // "((`a b))" |
Copilot
AI
Dec 10, 2025
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 expected output in the JSDoc comment is incorrect. When scope parameters are not provided (defaulting to empty strings), the result should include two spaces for the empty scopes. The output should be "(( \a b))"(with two spaces after the opening parentheses) instead of"((`a b))"`.
This matches the test expectation in tests/test_term.mjs line 109.
| * console.log(result.toString()); // "((`a b))" | |
| * console.log(result.toString()); // "(( `a b))" |
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.
Fixed in f9114ab. Updated to show "((r f `a b))" with hardcoded scopes "r" and "f".
tests/test_term.mjs
Outdated
| expect(result.toString()).toBe("((scope1 scope2 `a `b))"); | ||
| }); | ||
|
|
||
|
|
Copilot
AI
Dec 10, 2025
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.
[nitpick] There are two blank lines at the end of the file, but only one blank line is needed for consistency with the rest of the file and with the Python test file. Remove one of the extra blank lines.
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.
Fixed in f9114ab. Removed extra blank line at end of file.
apyds/term_t.py
Outdated
| >>> a = Term("`a") | ||
| >>> b = Term("b") | ||
| >>> result = a.match(b) | ||
| >>> str(result) if result else None # "((`a b))" |
Copilot
AI
Dec 10, 2025
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 expected output in the docstring example is incorrect. When scope parameters are not provided (defaulting to None, which are converted to empty strings), the result should include two spaces for the two empty scopes. The output should be "(( \a b))"(with two spaces after the opening parentheses) instead of"((`a b))"`.
This matches the test expectation in tests/test_term.py line 121.
| >>> str(result) if result else None # "((`a b))" | |
| >>> str(result) if result else None # "(( `a b))" |
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.
Fixed in f9114ab. Updated to show "((r f `a b))" with hardcoded scopes "r" and "f".
hzhangxyz
left a comment
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.
我对apyds和atsds进行了修改, 你帮我把其他部分处理了吧
- 检查apyds和atsds中是否存在笔误,如果有修复一下
- 接口变了, 所以tests和docs/api需要更新, 注意现在scope写死的,所以应该只需要simple complex fail三个情况了
- 最后需要使用pre commit 检查一下格式
- docs/concepts/下面term需要模仿rule对match添加一下介绍
Co-authored-by: hzhangxyz <11623447+hzhangxyz@users.noreply.github.com>
已完成所有更新 (commit f9114ab):
所有 Python 测试通过。 |
The C++
term_t::match()function existed but was not exposed in Python and TypeScript bindings. This adds the missing binding layer and API surface.Changes
Bindings (C++)
term_match()wrapper functions inapyds/ds.ccandatsds/ds.ccTermclass via pybind11/embindrule_t::matchbehavior)API Layer
Term.__matmul__(other)interm_t.py(using@operator)term_t.match(other)inindex.mtsTests & Docs
docs/api/docs/concepts/terms.mdUsage
Match returns dictionary as 4-tuple format:
((r f key value))with hardcoded scopes "r" and "f".Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.