Sample migration PR for librarian - test-20260408T102222#16584
Sample migration PR for librarian - test-20260408T102222#16584jskeet wants to merge 7 commits intogoogleapis:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request performs a large-scale migration of library configurations to librarian@latest, updates repository metadata across numerous packages, and adds significant new functionality to the Google Apps Chat and Memorystore clients. Specifically, it introduces support for managing sections and section items in Google Chat and adds shared regional certificate authority retrieval in Memorystore. Feedback identifies a critical syntax error in the synchronous Chat client where an await keyword was incorrectly used, a Python boolean literal error in the Django Spanner documentation, and an indentation inconsistency in the Chat client's parameter handling.
| # Wrap the RPC method; this adds retry and timeout information, | ||
| # and friendly error handling. | ||
| rpc = self._transport._wrapped_methods[self._transport.delete_section] | ||
|
|
There was a problem hiding this comment.
The delete_section method in the synchronous ChatServiceClient incorrectly uses the await keyword. This will result in a SyntaxError because the method is not defined as async.
rpc(| 'PROJECT': '$PROJECT', | ||
| 'INSTANCE': '$INSTANCE', | ||
| 'NAME': '$DATABASE', | ||
| 'RANDOM_ID_GENERATION_ENABLED': false, |
There was a problem hiding this comment.
In Python, the boolean literal for false is False (capitalized). Using false in this code snippet will cause a NameError if the code is executed.
| 'RANDOM_ID_GENERATION_ENABLED': false, | |
| 'RANDOM_ID_GENERATION_ENABLED': False, |
References
- Python boolean literals are capitalized (True, False). (link)
| # If we have keyword arguments corresponding to fields on the | ||
| # request, apply these. | ||
| if parent is not None: | ||
| request.parent = parent | ||
| if section is not None: | ||
| request.section = section |
There was a problem hiding this comment.
The application of flattened parameters is indented inside the if not isinstance(...) block. While functionally correct due to earlier checks, it is inconsistent with other methods in this file (e.g., list_sections) and with the ChatServiceAsyncClient where these assignments are outside the block. This inconsistency should be resolved for better maintainability.
| # If we have keyword arguments corresponding to fields on the | |
| # request, apply these. | |
| if parent is not None: | |
| request.parent = parent | |
| if section is not None: | |
| request.section = section | |
| # If we have keyword arguments corresponding to fields on the | |
| # request, apply these. | |
| if parent is not None: | |
| request.parent = parent | |
| if section is not None: | |
| request.section = section |
| @@ -1,5 +1,4 @@ | |||
| { | |||
| "api_description": "Pandas extension data types for data from SQL systems such as BigQuery.", | |||
There was a problem hiding this comment.
This change doesn't really matter as the README isn't generated from it.
| @@ -1 +1,288 @@ | |||
| .. include:: ../README.rst | |||
| Cloud Spanner support for Django | |||
There was a problem hiding this comment.
This is just because we're copying the parent readme file instead of using the include.
| @@ -1,5 +1,4 @@ | |||
| { | |||
| "api_description": "is a fully managed, NoOps, low cost data analytics service.\nData can be streamed into BigQuery at millions of rows per second to enable real-time analysis.\nWith BigQuery you can easily deploy Petabyte-scale Databases.", | |||
There was a problem hiding this comment.
Not sure why there's no description here, but againit doesn't appear to have changed a readme.
| "name_pretty": "iamconnectorcredentials.googleapis.com API", | ||
| "product_documentation": "https://cloud.google.com/iamconnectorcredentials/docs/overview", | ||
| "release_level": "preview", | ||
| "release_level": "stable", |
There was a problem hiding this comment.
This is a bug. Filed googleapis/librarian#5214
There was a problem hiding this comment.
This will be fixed - there's just nothing in sdk.yaml to say it's preview.
| "disable_auth": True, | ||
| "protocol": "protocol_value", | ||
| "overrides_by_request_protocol": {}, | ||
| "load_balancing_policy": "load_balancing_policy_value", |
There was a problem hiding this comment.
I don't know where the information in this file comes from. The API hasn't changed in a while. Filed googleapis/librarian#5215
There was a problem hiding this comment.
This is (I think) a bug in legacylibrarian or an operator error at some point. Regenerating explicitly with legacylibrarian creates the same change, so I think it wasn't regenerated when the image was updated. Unless there's any reason not to take the change, we should do so.
|
Closing as we're starting the migration. |
As ever, the last commit is the one to look at. (And the first commit wouldn't be present at all.)