-
Notifications
You must be signed in to change notification settings - Fork 117
docs: Update pub/sub quickstart to use background event #144
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
Conversation
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.
Thank you. This looks great! Just a few nits.
Hm, I tried it but was getting empty event
and context
fields. What did you do different? <- I was in different virtual envs for ff and pub/sub. Sorry about that!
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.
LGTM!
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.
LGTM, but 2 requested changes.
We usually refer to the signatures as event and cloudevent, where event has a data and context.
Using event and context may be confusing, as we have --signature-type=event
already.
So let's stick to data, context for this PR.
Google Cloud Functions [event](https://cloud.google.com/functions/docs/concepts/events-triggers#events) payloads to `event` and `context` objects. | ||
These will be passed as arguments to your function when it receives a request. | ||
Note that your function must use the `event`-style function signature: | ||
|
||
```python | ||
def hello(data, context): | ||
print(data) | ||
def hello(event, context): | ||
print(event) | ||
print(context) | ||
``` |
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.
Usually, we refer to the whole event as an event
, (like event and cloudevent), not the first param.
The event we refer to usually has two properties, data
, and context
. I suggest we keep the same terms here. This is like the docs are currently, and the Node Functions Framework. Although Java uses event.
If we want to use the term event, context, I suggest we do so in a separate PR and have a plan to update the docs at the same time.
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.
FWIW in most other language samples we use (message, context)
. I don't think it really matters much TBH
@@ -13,5 +13,5 @@ | |||
# limitations under the License. | |||
|
|||
|
|||
def hello(data, context): |
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 not update this identifier in this PR. Maybe a separate PR.
@@ -134,14 +134,14 @@ response instead. | |||
1. Create a `main.py` file with the following contents: | |||
|
|||
```python | |||
def hello(request): | |||
return "Hello world!" | |||
def hello(event, context): |
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 stick to data, context
to update this Pub/Sub quickstart. See other comments for more details.
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
@grant The change to |
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.
LGTM
@@ -134,14 +134,14 @@ response instead. | |||
1. Create a `main.py` file with the following contents: | |||
|
|||
```python | |||
def hello(request): | |||
return "Hello world!" | |||
def hello(event, context): |
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
Fixes #23, closes #140.