feat: add array __contains__ support #377
Conversation
Summary of ChangesHello @guan404ming, 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 enhances the Python Array class by adding native support for the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 the in operator (__contains__) to the Python Array class by introducing a C++ FFI function, ffi.ArrayContains. The overall approach is sound, and the Python-side implementation and tests are well-executed. I have a couple of suggestions to improve the C++ implementation and its corresponding test for better code quality and correctness.
| TEST(Array, Contains) { | ||
| Array<int> arr = {1, 2, 3, 4, 5}; | ||
| AnyEqual eq; | ||
|
|
||
| // Test element is present | ||
| bool found = false; | ||
| for (const auto& elem : *arr.GetArrayObj()) { | ||
| if (eq(elem, Any(3))) { | ||
| found = true; | ||
| break; | ||
| } | ||
| } | ||
| EXPECT_TRUE(found); | ||
|
|
||
| // Test element is not present | ||
| found = false; | ||
| for (const auto& elem : *arr.GetArrayObj()) { | ||
| if (eq(elem, Any(10))) { | ||
| found = true; | ||
| break; | ||
| } | ||
| } | ||
| EXPECT_FALSE(found); | ||
|
|
||
| // Test empty array | ||
| Array<int> empty_arr; | ||
| found = false; | ||
| for (const auto& elem : *empty_arr.GetArrayObj()) { | ||
| if (eq(elem, Any(1))) { | ||
| found = true; | ||
| break; | ||
| } | ||
| } | ||
| EXPECT_FALSE(found); | ||
|
|
||
| // Test with strings | ||
| Array<String> str_arr = {String("hello"), String("world")}; | ||
| found = false; | ||
| for (const auto& elem : *str_arr.GetArrayObj()) { | ||
| if (eq(elem, Any(String("world")))) { | ||
| found = true; | ||
| break; | ||
| } | ||
| } | ||
| EXPECT_TRUE(found); | ||
| } |
There was a problem hiding this comment.
The TEST(Array, Contains) test case currently re-implements the search logic manually. This means it's testing the search algorithm itself, rather than verifying that the ffi.ArrayContains function is correctly registered and behaves as expected.
To make this a more effective integration test, you should fetch the registered ffi.ArrayContains function from the FFI registry and call it directly. This will ensure the entire FFI pathway is working correctly.
Additionally, the test can be made more concise and readable by removing the repetitive found variable and manual loops.
TEST(Array, Contains) {
Function f = Function::Get("ffi.ArrayContains");
ASSERT_TRUE(f.defined());
Array<int> arr = {1, 2, 3, 4, 5};
EXPECT_TRUE(f(arr, 3));
EXPECT_TRUE(f(arr, 1));
EXPECT_TRUE(f(arr, 5));
EXPECT_FALSE(f(arr, 10));
EXPECT_FALSE(f(arr, 0));
Array<int> empty_arr;
EXPECT_FALSE(f(empty_arr, 1));
Array<String> str_arr = {String("hello"), String("world")};
EXPECT_TRUE(f(str_arr, String("hello")));
EXPECT_TRUE(f(str_arr, String("world")));
EXPECT_FALSE(f(str_arr, String("foo")));
}| .def("ffi.ArrayContains", | ||
| [](const ffi::ArrayObj* n, const Any& value) -> bool { | ||
| AnyEqual eq; | ||
| for (const Any& elem : *n) { | ||
| if (eq(elem, value)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| }) |
There was a problem hiding this comment.
The implementation of ffi.ArrayContains is correct, but it can be made more concise and idiomatic by using std::any_of from the <algorithm> header. This improves readability and aligns with modern C++ practices. The <algorithm> header is already available through existing includes.
.def("ffi.ArrayContains",
[](const ffi::ArrayObj* n, const Any& value) -> bool {
AnyEqual eq;
return std::any_of(n->begin(), n->end(),
[&](const Any& elem) { return eq(elem, value); });
})
2 participants
Why
Python arrays lack native in operator support, requiring manual iteration to check if a value exists.
How
ffi.ArrayContainsC++ FFI function__contains__method in Python Array class