-
Notifications
You must be signed in to change notification settings - Fork 218
Eg pubsub fixes #729
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
Eg pubsub fixes #729
Conversation
docgen/content-sources/toc.yaml
Outdated
| - title: 'TopicBuilder' | ||
| path: /docs/reference/functions/providers_pubsub_.topicbuilder.html | ||
| - title: 'ScheduleBuilder' | ||
| path: /docs/reference/functions/providers_pubsub_.schedulebuilder |
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.
Is this missing a .html at the end? I notice all the other have it
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.
Good catch; it works, but there's no reason it should stand out.
src/providers/pubsub.ts
Outdated
| } | ||
|
|
||
| /** | ||
| * The Google Cloud Pub/Sub schedule builder. |
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.
I hesitate to say Google Cloud Pub/Sub schedule builder. here, because that sounds to me like something that is built into pubsub. However, this is really just the Cloud Scheduler job that we create to run scheduled functions with.
Maybe this could be something like:
The schedule builder for scheduled functions. This describes the Cloud Scheduler job that will be deployed to trigger a scheduled function at the provided frequency.
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.
I know . . . I think the reason this was overlooked was exactly that: we thought it was a class outside of CF3.
But here it is -- sourced from providers/pubsub.ts and generated along with the other files.
Joe do you think we should we actually be hiding/omitting this class from the reference docs instead of trying to add it and flesh it out?
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.
We should definitely be documenting it as a part of the firebase-functions reference docs. What about:
"The builder for scheduled functions, which are powered by Pub/Sub and Cloud Scheduler."
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.
See what you think about this amalgam of all feedback. Snuck in a link to the guide.
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.
This version SGTM!
src/providers/pubsub.ts
Outdated
| } | ||
|
|
||
| /** | ||
| * Event handler that fires when a scheduled event occurs for |
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.
Language is a bit weird here - maybe something like "Event handler for scheduled functions. Triggered whenever the associated scheduler job sends a PubSub message"?
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.
+1
laurenzlong
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.
Describing scheduled functions is very tricky. Maybe we can try to be brief in the reference docs and link to https://firebase.google.com/docs/functions/schedule-functions whenever possible?
src/providers/pubsub.ts
Outdated
| } | ||
|
|
||
| /** | ||
| * The Google Cloud Pub/Sub schedule builder. |
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.
We should definitely be documenting it as a part of the firebase-functions reference docs. What about:
"The builder for scheduled functions, which are powered by Pub/Sub and Cloud Scheduler."
src/providers/pubsub.ts
Outdated
| } | ||
|
|
||
| /** | ||
| * Event handler that fires when a scheduled event occurs for |
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.
+1
|
Thanks for the feedback! Again, feel free to suggest commenting for anything else in pubsub.ts that should be described in the reference docs. |
src/providers/pubsub.ts
Outdated
| /** | ||
| * Registers a Cloud Function to run at specified times. | ||
| * | ||
| * @param schedule The scheduling logic, in Unix Crontab or AppEngine syntax. |
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.
I would go with the "The schedule" rather than "The scheduler logic", since it's not really a logic. It's just a definition of the schedule.
…ons into eg-pubsub-fixes
Adding ScheduleBuilder class to TOC and adding minimal commenting.
Happy to add additional comments for undocumented properties like retryConfig (or hide them if necessary); just LMK the right general verbiage.
Staged internally at: https://firebase.devsite.corp.google.com/docs/reference/functions/providers_pubsub_.schedulebuilder.
Thanks!