[test] - #18081
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the google-cloud-language package across versions v1, v1beta2, and v2 by introducing a shared _compat.py module to handle fallback logic for older versions of google-api-core. It simplifies REST transports by delegating request transcoding to this new module, removes obsolete manual Python and dependency version checks, and bumps the minimum required google-api-core version to 2.28.0. Feedback on the newly added unit tests highlights a style guide violation where path_template.transcode is mocked globally instead of using its localized module import path.
| with mock.patch( | ||
| "google.api_core.path_template.transcode", | ||
| return_value={"method": "get", "uri": "/v1/test"}, | ||
| ): |
There was a problem hiding this comment.
According to the repository style guide, standard functions or external helpers should be mocked using their local module import path rather than patching them globally. This ensures that mocks remain isolated and do not leak or affect other tests.
Please update the mock patch to target the local import path google.cloud.language_v1._compat.path_template.transcode.
| with mock.patch( | |
| "google.api_core.path_template.transcode", | |
| return_value={"method": "get", "uri": "/v1/test"}, | |
| ): | |
| with mock.patch( | |
| "google.cloud.language_v1._compat.path_template.transcode", | |
| return_value={"method": "get", "uri": "/v1/test"}, | |
| ): |
References
- When mocking standard functions or filesystem checks, mock the local module import path instead of patching globally, ensuring mocks are isolated. (link)
| with mock.patch( | ||
| "google.api_core.path_template.transcode", | ||
| return_value={"method": "get", "uri": "/v1/test"}, | ||
| ): |
There was a problem hiding this comment.
According to the repository style guide, standard functions or external helpers should be mocked using their local module import path rather than patching them globally. This ensures that mocks remain isolated and do not leak or affect other tests.
Please update the mock patch to target the local import path google.cloud.language_v1beta2._compat.path_template.transcode.
| with mock.patch( | |
| "google.api_core.path_template.transcode", | |
| return_value={"method": "get", "uri": "/v1/test"}, | |
| ): | |
| with mock.patch( | |
| "google.cloud.language_v1beta2._compat.path_template.transcode", | |
| return_value={"method": "get", "uri": "/v1/test"}, | |
| ): |
References
- When mocking standard functions or filesystem checks, mock the local module import path instead of patching globally, ensuring mocks are isolated. (link)
| with mock.patch( | ||
| "google.api_core.path_template.transcode", | ||
| return_value={"method": "get", "uri": "/v1/test"}, | ||
| ): |
There was a problem hiding this comment.
According to the repository style guide, standard functions or external helpers should be mocked using their local module import path rather than patching them globally. This ensures that mocks remain isolated and do not leak or affect other tests.
Please update the mock patch to target the local import path google.cloud.language_v2._compat.path_template.transcode.
| with mock.patch( | |
| "google.api_core.path_template.transcode", | |
| return_value={"method": "get", "uri": "/v1/test"}, | |
| ): | |
| with mock.patch( | |
| "google.cloud.language_v2._compat.path_template.transcode", | |
| return_value={"method": "get", "uri": "/v1/test"}, | |
| ): |
References
- When mocking standard functions or filesystem checks, mock the local module import path instead of patching globally, ensuring mocks are isolated. (link)
No description provided.