[DRAFT] feat(firestore): add new array-based pipeline expressions#16148
[DRAFT] feat(firestore): add new array-based pipeline expressions#16148daniel-sanche wants to merge 3 commits intomainfrom
Conversation
Summary of ChangesHello, 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 enhances the Firestore client library by introducing a suite of new array-based pipeline expressions. These additions provide developers with more powerful and flexible tools for manipulating and querying array data directly within Firestore, streamlining common operations like finding min/max elements, slicing, and indexing. 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. Footnotes
|
There was a problem hiding this comment.
Code Review
The pull request introduces several new array manipulation functions, including array_maximum, array_minimum, array_first, array_last, array_slice, and their _n variants, along with array_index_of and array_index_of_all, to pipeline_expressions.py, complete with corresponding unit and system tests. Review feedback indicates that the unit test for array_index_of is missing an assertion for its third parameter, and the array_index_of function itself should replace a hardcoded string with a constant for improved readability and maintainability.
| assert len(instance.params) == 3 | ||
| assert instance.params[0] == arg1 | ||
| assert instance.params[1] == arg2 | ||
| infix_instance = arg1.array_index_of(arg2) |
There was a problem hiding this comment.
The test test_array_index_of only checks that instance.params[0] is arg1 and instance.params[1] is arg2. It does not check that instance.params[2] is the constant "first". This check should be added to ensure the function is working as expected.
| assert len(instance.params) == 3 | |
| assert instance.params[0] == arg1 | |
| assert instance.params[1] == arg2 | |
| infix_instance = arg1.array_index_of(arg2) | |
| assert instance.name == "array_index_of" | |
| assert len(instance.params) == 3 | |
| assert instance.params[0] == arg1 | |
| assert instance.params[1] == arg2 | |
| infix_instance = arg1.array_index_of(arg2) |
Adds the following expressions:
TODO: Make sure docstrings and arguments align with other languages