Ppl public API changes #15066
Ppl public API changes #15066
Conversation
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) 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 is currently in preview and 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 to provide feedback. |
Generated by 🚫 Danger |
|
/gemini review |
|
/gemini summary |
![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 introduces significant and largely positive changes to the public Pipeline API. Key improvements include renaming Expr to Expression and its related types for better clarity, adopting more idiomatic Swift method names (e.g., greaterThan instead of gt), and consistently using arrays instead of variadic parameters for better API design. The documentation has also been substantially improved. I've identified a few minor areas for improvement, mainly around API convenience and documentation consistency.
| /// Creates an expression that concatenates this string expression with other string expressions. | ||
| /// Assumes `self` evaluates to a string. | ||
| /// Assumes `self` and all parameters evaluate to strings. | ||
| /// | ||
| /// ```swift | ||
| /// // Combine 'part1', 'part2', and 'part3' fields | ||
| /// Field("part1").strConcat(Field("part2"), Field("part3")) | ||
| /// // Combine "firstName", "middleName", and "lastName" fields | ||
| /// Field("firstName").strConcat(Field("middleName"), Field("lastName")) | ||
| /// ``` | ||
| /// | ||
| /// - Parameter secondString: An `Expr` (evaluating to a string) to concatenate. | ||
| /// - Parameter otherStrings: Optional additional `Expr` (evaluating to strings) to concatenate. | ||
| /// - Returns: A new `FunctionExpr` representing the concatenated string. | ||
| func strConcat(_ secondString: Expr, _ otherStrings: Expr...) -> FunctionExpr | ||
|
|
||
| /// Creates an expression that concatenates this string expression with other string literals. | ||
| /// Assumes `self` evaluates to a string. | ||
| /// | ||
| /// ```swift | ||
| /// // Combine 'firstName', " ", and 'lastName' | ||
| /// Field("firstName").strConcat(" ", "lastName") | ||
| /// ``` | ||
| /// | ||
| /// - Parameter secondString: A string literal to concatenate. | ||
| /// - Parameter otherStrings: Optional additional string literals to concatenate. | ||
| /// - Returns: A new `FunctionExpr` representing the concatenated string. | ||
| func strConcat(_ secondString: String, _ otherStrings: String...) -> FunctionExpr | ||
| /// - Parameter secondString: An `Expression` (evaluating to a string) to concatenate. | ||
| /// - Parameter otherStrings: Optional additional `Expression` (evaluating to strings) to | ||
| /// concatenate. | ||
| /// - Returns: A new `FunctionExpression` representing the concatenated string. | ||
| func strConcat(_ strings: [Expression]) -> FunctionExpression |
There was a problem hiding this comment.
The strConcat method has been updated to take an array of Expressions, which improves consistency. However, the previous convenience overload for string literals (_ secondString: String, _ otherStrings: String...) has been removed. This makes concatenating with literals more verbose (e.g., strConcat([Constant("a"), Constant("b")]) instead of strConcat("a", "b")). Please consider reintroducing an overload for String literals to improve developer experience. For example:
func strConcat(_ strings: String...) -> FunctionExpressionThere was a problem hiding this comment.
Discussed. Not using variadic parameter in API due to its poor compatibility with array in Swift.
| public func aggregate(_ aggregates: [AliasedAggregate], | ||
| groups: [Selectable]? = nil) -> Pipeline { | ||
| return Pipeline(stages: stages + [Aggregate(accumulators: accumulator, groups: groups)], db: db) | ||
| } | ||
|
|
||
| /// Performs optionally grouped aggregation operations using field names for grouping. | ||
| /// | ||
| /// Similar to the other `aggregate` method, but `groups` are specified as an array of `String` | ||
| /// field names. | ||
| /// | ||
| /// ```swift | ||
| /// // let pipeline: Pipeline = ... // Assume pipeline from "books" collection. | ||
| /// // Count books for each publisher. | ||
| /// let groupedByPublisherPipeline = pipeline.aggregate( | ||
| /// [AggregateWithas(aggregate: countAll(), alias: "book_count")], | ||
| /// groups: ["publisher"] // Group by the "publisher" field name. | ||
| /// ) | ||
| /// // let results = try await groupedByPublisherPipeline.execute() | ||
| /// // results.documents might be: | ||
| /// // [ | ||
| /// // ["publisher": "Penguin", "book_count": 50], | ||
| /// // ["publisher": "HarperCollins", "book_count": 35] | ||
| /// // ] | ||
| /// ``` | ||
| /// | ||
| /// - Parameters: | ||
| /// - accumulator: An array of `AggregateWithAlias` expressions. | ||
| /// - groups: An optional array of `String` field names for grouping. | ||
| /// - Returns: A new `Pipeline` object with this stage appended. | ||
| public func aggregate(_ accumulator: [AggregateWithAlias], | ||
| groups: [String]? = nil) -> Pipeline { | ||
| let selectables = groups?.map { Field($0) } | ||
| return Pipeline( | ||
| stages: stages + [Aggregate(accumulators: accumulator, groups: selectables)], | ||
| db: db | ||
| ) | ||
| return Pipeline(stages: stages + [Aggregate(accumulators: aggregates, groups: groups)], db: db) | ||
| } |
There was a problem hiding this comment.
The aggregate method overload that accepted an array of Strings for grouping has been removed. This was a convenient API for simple grouping. Now, developers must explicitly create Field objects (e.g., groups: [Field("genre")] instead of groups: ["genre"]). Please consider adding this overload back for improved usability. A possible implementation could be:
public func aggregate(_ aggregates: [AliasedAggregate],
groups: [String]? = nil) -> Pipeline {
let selectables = groups?.map { Field($0) }
return self.aggregate(aggregates, groups: selectables)
}There was a problem hiding this comment.
The reason for removing it is because if the optional field is omit, like the function only takes in first parameter, including overloads will make the call ambiguous since both overloads fulfill the syntax.
| /// - Parameter other: An array of at least one `Pipeline` whose documents will be unioned. | ||
| /// - Returns: A new `Pipeline` object with this stage appended. |
There was a problem hiding this comment.
The documentation for union(with:) states that the other parameter is an array of Pipelines, but the function signature accepts a single Pipeline. Please correct the documentation to match the implementation.
| // TBD | ||
| // _ = db.pipeline().collection("books") | ||
| // .addFields( | ||
| // FieldPath.documentID() | ||
| // ) |
There was a problem hiding this comment.
This section is commented out with a "TBD" note. Please ensure this is either implemented or removed before merging.
Summary of ChangesThis pull request introduces a significant refactoring of the public API for Firestore's pipeline feature, focusing on enhancing consistency, clarity, and Swift idiomatic usage. The changes streamline how expressions are built and how pipeline stages are configured, aiming to provide a more intuitive and predictable developer experience. Highlights
Changelog
Activity
|
No description provided.