-
Notifications
You must be signed in to change notification settings - Fork 161
feat: pipelines preview #2450
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
base: main
Are you sure you want to change the base?
feat: pipelines preview #2450
Conversation
… in expression.ts
|
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
|
Warning: This pull request is touching the following templated files:
|
wu-hui
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.
Only skimmed through the tests, and left some minor nits.
Nothing is blocking, other than the version string needs to be corrected in package.json
| // @beta | ||
| class AggregateFunction implements AggregateFunction, HasUserData { | ||
| constructor(name: string, params: Expression[]); | ||
| // Warning: (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen |
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.
Just to confirm, these warnings will be addressed in a followup PR?
| const RESOURCE_PATH_RE = | ||
| // Note: [\s\S] matches all characters including newlines. | ||
| /^projects\/([^/]*)\/databases\/([^/]*)(?:\/documents\/)?([\s\S]*)$/; | ||
| /^projects\/([^/]+)\/databases\/([^/]+)(?:\/documents(?:\/([^/]+(?:\/[^/]+)*))?)?$/; |
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.
Can you remind me why the RE change? I think I reviewed this but now cannot recall the context.
| nestedOptions?: OptionsDefinitions; | ||
| }; | ||
|
|
||
| export class OptionsUtil { |
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.
Non-blocking: Maybe add some comments in this file, it is hard to read and understand what each method does.
| }; | ||
| }; | ||
|
|
||
| export class StructuredPipeline |
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.
Maybe explain why this is desired?
| "name": "@google-cloud/firestore", | ||
| "description": "Firestore Client Library for Node.js", | ||
| "version": "8.0.0", | ||
| "version": "8.1.0-eap-pipelines.2", |
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.
Think we don't need to do this for public preview?
| import {OptionsUtil} from './options-util'; | ||
| import {CollectionReference} from '../reference/collection-reference'; | ||
|
|
||
| /** |
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.
Fill the comments in this file, not blocking though.
No description provided.