feat(firestore): add rand and trunc expressions#9498
Conversation
🦋 Changeset detectedLatest commit: 767925b The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello @yvonnep165, 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 Firestore's pipeline capabilities by introducing two new arithmetic expressions: 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. Changelog
Activity
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
|
There was a problem hiding this comment.
Code Review
This pull request introduces rand and trunc arithmetic expressions to Firestore pipelines, complete with implementations, comprehensive tests, and documentation updates. The code is well-structured and follows existing patterns. My main feedback relates to improving the clarity of the generated documentation by consolidating JSDoc comments for overloaded functions, for which I've provided specific suggestions.
| trunc(): FunctionExpression; | ||
|
|
||
| /** | ||
| * @beta | ||
| * Creates an expression that truncates a numeric value to the specified number of decimal places. | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * // Truncate the value of the 'rating' field to two decimal places. | ||
| * field("rating").trunc(2); | ||
| * ``` | ||
| * | ||
| * @param decimalPlaces - A constant specifying the truncation precision in decimal places. | ||
| * | ||
| * @returns A new `Expression` representing the truncated value. | ||
| */ | ||
| trunc(decimalPlaces: number): FunctionExpression; | ||
|
|
||
| /** | ||
| * @beta | ||
| * Creates an expression that truncates a numeric value to the specified number of decimal places. | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * // Truncate the value of the 'rating' field to two decimal places. | ||
| * field("rating").trunc(constant(2)); | ||
| * ``` | ||
| * | ||
| * @param decimalPlaces - An expression specifying the truncation precision in decimal places. | ||
| * | ||
| * @returns A new `Expression` representing the truncated value. | ||
| */ | ||
| trunc(decimalPlaces: Expression): FunctionExpression; |
There was a problem hiding this comment.
The documentation for the trunc method overloads is split across multiple JSDoc blocks, leading to verbose and somewhat redundant generated documentation. To improve clarity for developers using the API, I recommend consolidating the documentation for the overloads that accept decimalPlaces into a single JSDoc block.
For example:
/**
* @beta
* Creates an expression that truncates a numeric value to the specified number of decimal places.
*
* @example
* ```typescript
* // Truncate the value of the 'rating' field to two decimal places.
* field("rating").trunc(2);
*
* // Truncate the value of the 'rating' field to the number of decimal places
* // specified in the 'precision' field.
* field("rating").trunc(field("precision"));
* ```
*
* @param decimalPlaces - A constant or expression specifying the truncation precision in decimal places.
*
* @returns A new `Expression` representing the truncated value.
*/
trunc(decimalPlaces: number | Expression): FunctionExpression;This would require adjusting the function signatures to something like the following, which would improve the generated documentation for trunc and could be a good pattern to apply to other methods like round in the future.
trunc(): FunctionExpression;
trunc(decimalPlaces: number | Expression): FunctionExpression;
trunc(decimalPlaces?: number | Expression): FunctionExpression { /* ... */ }There was a problem hiding this comment.
You can ignore this comment. This is a pattern done in the other expressions.
| /** | ||
| * @beta | ||
| * Creates an expression that truncates the numeric value of a field to an integer. | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * // Truncate the value of the 'rating' field | ||
| * trunc("rating"); | ||
| * ``` | ||
| * | ||
| * @param fieldName - The name of the field containing the number to truncate. | ||
| * @returns A new `Expression` representing the truncated value. | ||
| */ | ||
| export function trunc(fieldName: string): FunctionExpression; | ||
|
|
||
| /** | ||
| * @beta | ||
| * Creates an expression that truncates the numeric value of an expression to an integer. | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * // Truncate the value of the 'rating' field. | ||
| * trunc(field("rating")); | ||
| * ``` | ||
| * | ||
| * @param expression - An expression evaluating to a numeric value, which will be truncated. | ||
| * @returns A new `Expression` representing the truncated value. | ||
| */ | ||
| export function trunc(expression: Expression): FunctionExpression; | ||
|
|
||
| /** | ||
| * @beta | ||
| * Creates an expression that truncates a numeric expression to the specified number of decimal places. | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * // Truncate the value of the 'rating' field to two decimal places. | ||
| * trunc("rating", 2); | ||
| * ``` | ||
| * | ||
| * @param fieldName - The name of the field to truncate. | ||
| * @param decimalPlaces - A constant or expression specifying the truncation precision in decimal places. | ||
| * @returns A new `Expression` representing the truncated value. | ||
| */ | ||
| export function trunc( | ||
| fieldName: string, | ||
| decimalPlaces: number | Expression | ||
| ): FunctionExpression; | ||
|
|
||
| /** | ||
| * @beta | ||
| * Creates an expression that truncates a numeric value to the specified number of decimal places. | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * // Truncate the value of the 'rating' field to two decimal places. | ||
| * trunc(field("rating"), constant(2)); | ||
| * ``` | ||
| * | ||
| * @param expression - An expression evaluating to a numeric value, which will be truncated. | ||
| * @param decimalPlaces - A constant or expression specifying the truncation precision in decimal places. | ||
| * @returns A new `Expression` representing the truncated value. | ||
| */ | ||
| export function trunc( | ||
| expression: Expression, | ||
| decimalPlaces: number | Expression | ||
| ): FunctionExpression; |
There was a problem hiding this comment.
Similar to my comment on Expression.trunc(), the top-level trunc() function has multiple overloads with duplicated JSDoc comments, which makes the generated documentation verbose. Consolidating these would improve readability.
Here's a suggestion for how you could combine them:
/**
* @beta
* Creates an expression that truncates a numeric value to an integer.
*
* @example
* ```typescript
* // Truncate the value of the 'rating' field.
* trunc("rating");
* trunc(field("rating"));
* ```
*
* @param expression - A field name or an expression evaluating to a numeric value.
* @returns A new `Expression` representing the truncated value.
*/
export function trunc(expression: string | Expression): FunctionExpression;
/**
* @beta
* Creates an expression that truncates a numeric value to the specified number of decimal places.
*
* @example
* ```typescript
* // Truncate the value of the 'rating' field to two decimal places.
* trunc("rating", 2);
* trunc(field("rating"), constant(2));
* ```
*
* @param expression - A field name or an expression evaluating to a numeric value.
* @param decimalPlaces - A constant or expression specifying the truncation precision.
* @returns A new `Expression` representing the truncated value.
*/
export function trunc(
expression: string | Expression,
decimalPlaces: number | Expression
): FunctionExpression;This would produce a much cleaner and more user-friendly documentation page.
| trunc(): FunctionExpression; | ||
|
|
||
| /** | ||
| * @beta | ||
| * Creates an expression that truncates a numeric value to the specified number of decimal places. | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * // Truncate the value of the 'rating' field to two decimal places. | ||
| * field("rating").trunc(2); | ||
| * ``` | ||
| * | ||
| * @param decimalPlaces - A constant specifying the truncation precision in decimal places. | ||
| * | ||
| * @returns A new `Expression` representing the truncated value. | ||
| */ | ||
| trunc(decimalPlaces: number): FunctionExpression; | ||
|
|
||
| /** | ||
| * @beta | ||
| * Creates an expression that truncates a numeric value to the specified number of decimal places. | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * // Truncate the value of the 'rating' field to two decimal places. | ||
| * field("rating").trunc(constant(2)); | ||
| * ``` | ||
| * | ||
| * @param decimalPlaces - An expression specifying the truncation precision in decimal places. | ||
| * | ||
| * @returns A new `Expression` representing the truncated value. | ||
| */ | ||
| trunc(decimalPlaces: Expression): FunctionExpression; |
There was a problem hiding this comment.
You can ignore this comment. This is a pattern done in the other expressions.
Co-authored-by: Daniel La Rocque <dlarocque@google.com>
Co-authored-by: Daniel La Rocque <dlarocque@google.com>
Co-authored-by: Daniel La Rocque <dlarocque@google.com>
|
Discussed offline. Since rand() takes 0 arguments and produces a value from "nothing" it does not transform an existing expression (for example, field("rating").rand() is confusing and meaningless. So it will only exist as a standalone function and not in the |
Add new arithmetic expressions
randandtruncand integration tests for the two expressions