-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
basic python search example for docs #13119
base: main
Are you sure you want to change the base?
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.
Hi team,
I'm currently reviewing this pull request and will provide my feedback shortly. In the meantime, here's a summary for you and other reviewers to quickly get up to speed:
This pull request adds a basic Python search example to the documentation. The intent is to provide a clear, concise example of how to use the Google Cloud Retail Search API.
Here's a breakdown of the changes:
- File Changed:
retail/doc-samples/search.py
- Changes: This file contains a new Python function,
search_products
, which demonstrates how to perform a search using thegoogle.cloud.retail_v2
library. The function takes several parameters, including project ID, catalog ID, placement ID, branch, visitor ID, and the search query. It then constructs aSearchRequest
, sends it to the API, and prints the response. The file also includes example usage with placeholder values for the required parameters.
The code is well-structured and easy to follow. The use of clear variable names and comments enhances readability. The example clearly shows how to use the key methods and parameters of the Retail Search API.
I'll be looking at the overall clarity, error handling, and best practices in my full review.
And now, a little haiku to brighten your day:
Code flows like a stream,
Clear examples guide the way,
Docs shine, bright and new.
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 pull request adds a basic Python search example. This is a good start, but there are some improvements that can be made to enhance readability, maintainability, and correctness.
I've summarized the style guide elements used below:
- Descriptive Function and Parameter Names: Function and parameter names should be descriptive, even if they are long. (See Authoring Guide)
- Type Hinting: Argument types should be documented using Python type annotations. (See Authoring Guide)
- Docstrings: Functions should have docstrings explaining their purpose and usage. (See Authoring Guide)
- Example Usage: Example usage should be clear and runnable, with placeholders for user-specific values. (See Authoring Guide)
- README File: Each sample should have a README.md file with instructions for installation, configuration, and execution. (See Authoring Guide)
license notice Co-authored-by: Holt Skinner <[email protected]>
# Example usage | ||
project_id = 'YOUR_PROJECT_ID' | ||
catalog_id = 'YOUR_CATALOG_ID' | ||
placement_id = 'YOUR_PLACEMENT_ID' | ||
branch = 'YOUR_BRANCH_NAME' | ||
visitor_id = 'YOUR_VISITOR_ID' | ||
query = 'YOUR_QUERY' | ||
|
||
search_products(project_id, catalog_id, placement_id, branch, visitor_id, query) |
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 sample does not match our styleguide, particularly with respect to testing: https://googlecloudplatform.github.io/samples-style-guide/#testing
You will need to also add a region tag around the part of the code you want to include in the documentation.
I would also suggest you move this sample to a different folder, as doc-samples
isn't very clear. I would suggest retail/snippets/search_request.py
, and you can then create retail/snippets/search_request_test.py
for the test. You can also use this folder for future samples with a shared requirements.txt
another configs (which you have also missed)
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.
Please look over the previous code review and address the issues raised.
Description
Fixes #268120562