-
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
chore(job): rename and delete tags at jobs/v3/api_client/location_search_sample.py #13005
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 have a full review ready in a few minutes. In the meantime, here's a quick summary to help you and other reviewers get up to speed:
This pull request renames several tags within the jobs/v3/api_client/location_search_sample.py
file. The intent, as I understand it from the PR description and commit messages, is to fix several internal bugs (b/384774431, b/347825997, b/384780822, b/347825791, b/347825792) related to documentation renaming (cl/648538021). Specifically, the changes involve renaming tags associated with location search functions. The old tags (e.g., [START basic_location_search]
, [END basic_location_search]
) are replaced with new tags that include the prefix "job" (e.g., [START job_basic_location_search]
, [END job_basic_location_search]
). This change affects multiple functions within the file: basic_location_search
, keyword_location_search
, city_location_search
, multi_locations_search
, and broadening_location_search
. All instances of the old tags are consistently updated to their new counterparts.
If I've missed anything crucial, please let me know. I'll provide a more detailed review shortly.
A little poem for your coding pleasure:
The code compiles,
No errors, a joyful sight,
Review now begins.
Best,
Gemini
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 correctly renames tags related to location search within the jobs/v3/api_client/location_search_sample.py
file. The changes accurately reflect the updated documentation and maintain consistency. The commit message is clear, concise, and informative, properly linking to the relevant bug fixes. The inclusion of the checklist ensures compliance with project guidelines. Overall, the pull request demonstrates good attention to detail and adherence to best practices.
@@ -25,7 +25,7 @@ | |||
# [END instantiate] | |||
|
|||
|
|||
# [START basic_location_search] | |||
# [START job_basic_location_search] |
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.
Correctly renamed the [START]
tag for the basic_location_search
function. Adding a concise in-line comment explaining the function's purpose and parameters would improve readability. For example:
# Performs a basic location search for jobs using the provided client service, company name, location, and distance.
@@ -47,10 +47,10 @@ def basic_location_search(client_service, company_name, location, distance): | |||
print(response) | |||
|
|||
|
|||
# [END basic_location_search] | |||
# [END job_basic_location_search] |
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.
The [END]
tag for basic_location_search
is correctly updated to match the new name. Ensure all region tags within the project are consistently updated.
|
||
|
||
# [START keyword_location_search] | ||
# [START job_keyword_location_search] |
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.
The [START]
tag for keyword_location_search
is accurately renamed. A brief explanation of how this function incorporates keyword search would be helpful. For example:
# Performs a keyword-based location search, adding a keyword filter to the job query.
@@ -72,10 +72,10 @@ def keyword_location_search(client_service, company_name, location, distance, ke | |||
print(response) | |||
|
|||
|
|||
# [END keyword_location_search] | |||
# [END job_keyword_location_search] |
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.
Correctly renamed the [END]
tag for keyword_location_search
. Maintaining consistent and accurate region tags is crucial for documentation.
|
||
|
||
# [START city_location_search] | ||
# [START job_city_location_search] |
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.
The [START]
tag for city_location_search
is correctly updated. Adding a short comment about the city-specific search logic would enhance clarity. For example:
# Performs a location search limited to a specific city.
@@ -97,10 +97,10 @@ def city_location_search(client_service, company_name, location): | |||
print(response) | |||
|
|||
|
|||
# [END city_location_search] | |||
# [END job_city_location_search] |
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.
Accurately renamed the [END]
tag for city_location_search
. Well-defined region tags contribute to well-structured documentation.
|
||
|
||
# [START multi_locations_search] | ||
# [START job_multi_locations_search] |
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.
Correctly renamed the [START]
tag for multi_locations_search
. Consider adding a comment explaining the handling of multiple locations. For example:
# Performs a job search using multiple location filters.
@@ -125,10 +125,10 @@ def multi_locations_search( | |||
print(response) | |||
|
|||
|
|||
# [END multi_locations_search] | |||
# [END job_multi_locations_search] |
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.
The [END]
tag for multi_locations_search
is correctly updated. Consistent naming and placement of region tags improve code maintainability.
|
||
|
||
# [START broadening_location_search] | ||
# [START job_broadening_location_search] |
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.
The [START]
tag for broadening_location_search
is accurately renamed. A comment clarifying the broadening search behavior would be valuable. For example:
# Performs a location search with broadening enabled, expanding the search radius if initial results are limited.
@@ -151,7 +151,7 @@ def broadening_location_search(client_service, company_name, location): | |||
print(response) | |||
|
|||
|
|||
# [END broadening_location_search] | |||
# [END job_broadening_location_search] |
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.
The [END]
tag for broadening_location_search
correctly reflects the function name. Consistent use of region tags aids in clear documentation generation.
Here is the summary of possible violations 😱 There are 3 possible violations for removing region tag in use.
The end of the violation section. All the stuff below is FYI purposes only. Here is the summary of changes. You are about to add 5 region tags.
You are about to delete 6 region tags.
This comment is generated by snippet-bot.
|
I see that the source code for documents is already updated for fixed regions. |
|
||
|
||
# [START basic_location_search] | ||
# [START job_basic_location_search] |
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.
Renaming region tags need to have the new region tag included, then updating the docs, then removing the old tag.
# [START job_basic_location_search] | |
# [START basic_location_search] | |
# [START job_basic_location_search] |
(Change this in all other places. This is an earlier PR from you, you may have already adapted to this pattern since)
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.
Thanks glasnt!
Please check b/347825791 for more details on this example basic_location_search
.
Documentation has already been fixed by yoshifumi in July 2024. (CL referenced in the Bug)
Even in this case, should I add both region tags? job_basic_location_search
and basic_location_search
Description
Fixes Internal:
b/384774431
b/347825997
b/384780822
b/347825791
b/347825792
b/347350338
Regions renaming in documentation was made in cl/648538021
Checklist
nox -s py-3.9
(see Test Environment Setup)nox -s lint
(see Test Environment Setup)