Skip to content

Conversation

@grant
Copy link
Contributor

@grant grant commented Jun 9, 2021

Documents the base request framework used for this library.

Fixes #80
R @anniefu

Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>
@grant grant self-assigned this Jun 9, 2021
@google-cla google-cla bot added the cla: yes label Jun 9, 2021
@di
Copy link
Member

di commented Jun 9, 2021

I don't think this is sufficient to fix #80? That issue is about documenting the type of the request parameter. Linking to the Flask docs is a sort of roundabout way to do it, I'm not sure that would be clear to the average user that this means that the request parameter is a Flask Request object.

@grant
Copy link
Contributor Author

grant commented Jun 9, 2021

I don't think this is sufficient to fix #80? That issue is about documenting the type of the request parameter. Linking to the Flask docs is a sort of roundabout way to do it, I'm not sure that would be clear to the average user that this means that the request parameter is a Flask Request object.

Do you have an idea of how we can fix this?

Should we link this instead?

https://flask.palletsprojects.com/en/1.1.x/api/#flask.request

@di
Copy link
Member

di commented Jun 10, 2021

In #80 I pointed out that we already do this at https://cloud.google.com/functions/docs/concepts/events-triggers#functions_parameters-python, which says:

Your function is passed a single parameter, (request), which is a Flask Request object.

We could copy/paste this into the docs here, or we could just link to https://cloud.google.com/functions/docs/concepts/events-triggers#functions_parameters-python which also documents this for background functions as well.

@grant
Copy link
Contributor Author

grant commented Jun 10, 2021

In #80 I pointed out that we already do this at cloud.google.com/functions/docs/concepts/events-triggers#functions_parameters-python, which says:

Your function is passed a single parameter, (request), which is a Flask Request object.

We could copy/paste this into the docs here, or we could just link to cloud.google.com/functions/docs/concepts/events-triggers#functions_parameters-python which also documents this for background functions as well.

Yes, Cloud Functions does talk about this in more detail. However, the Function Frameworks are supposed to be runnable outside of Cloud Functions. I think it's not reasonable to expect users to go find that page, and it's reasonable to talk about the fact Flask is used, especially for new developers. Ruby, Node, other languages state info like this as well.

@anniefu
Copy link
Contributor

anniefu commented Jun 10, 2021

For me, I'm happy as long as the Flask Request is documented. My two cents here is that we should copy/paste the Flask request bit into this repo.

At the end of the day, Functions Framework decides what the request format is, and the Google Cloud Docs get updated to match, so this repo should be the most up-to-date source of truth.

@grant
Copy link
Contributor Author

grant commented Jun 15, 2021

@di Can you suggest an alternative way to mention Flask for the user's function handler in the README?

@di
Copy link
Member

di commented Jun 15, 2021

In #134 (comment) I said that we could copy/paste the following into the docs here from the Cloud Functions docs:

Your function is passed a single parameter, (request), which is a Flask Request object.

Is there any reason that we don't want to do that?

Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>
@grant
Copy link
Contributor Author

grant commented Jun 22, 2021

In #134 (comment) I said that we could copy/paste the following into the docs here from the Cloud Functions docs:

Your function is passed a single parameter, (request), which is a Flask Request object.

Is there any reason that we don't want to do that?

That's fine. I think I've update the PR to fit your suggestion.

@grant grant requested a review from di June 22, 2021 15:44
Copy link
Member

@di di left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, not sure why conformance tests are now failing though.

@di di merged commit 54d87d0 into master Jul 20, 2021
@di di deleted the grant_docs_flask branch July 20, 2021 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Document incoming request type and/or add request parsing examples

4 participants