chore: migrate packages from black to ruff - #18135
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the linting, import sorting, and formatting toolchain from Black to Ruff across multiple packages, resulting in widespread import reordering and minor formatting adjustments. The review feedback highlights several violations of the repository's style guide regarding localized mocking, specifically where os.path.exists is patched globally in test suites instead of mocking the local module import path. Additionally, an unnecessary parenthesized expression with a trailing comma in test_aws.py was flagged for creating an unused 1-tuple.
| ) as mock_session: | ||
| with ( | ||
| mock.patch.dict(os.environ, {"GOOGLE_API_USE_CLIENT_CERTIFICATE": "true"}), | ||
| mock.patch("os.path.exists") as mock_exists, |
There was a problem hiding this comment.
Patching os.path.exists globally violates the repository style guide's localized mocking rule. Instead, mock the local module import path google.auth.transport._mtls_helper.path.exists to ensure mocks are isolated and do not cause global side effects.
| mock.patch("os.path.exists") as mock_exists, | |
| mock.patch("google.auth.transport._mtls_helper.path.exists") as mock_exists, |
References
- Localized Mocking: When mocking standard functions or filesystem checks (like path.exists), mock the local module import path instead of patching builtins globally, ensuring mocks are isolated. (link)
| with mock.patch.object(os.path, "exists", return_value=True), mock.patch( | ||
| "builtins.open", mock.mock_open() | ||
| with ( | ||
| mock.patch.object(os.path, "exists", return_value=True), |
There was a problem hiding this comment.
Patching os.path.exists globally via mock.patch.object violates the repository style guide's localized mocking rule. Please mock the local module import path google.auth.transport._mtls_helper.os.path.exists instead to keep the mock isolated.
| mock.patch.object(os.path, "exists", return_value=True), | |
| mock.patch("google.auth.transport._mtls_helper.os.path.exists", return_value=True), |
References
- Localized Mocking: When mocking standard functions or filesystem checks (like path.exists), mock the local module import path instead of patching builtins globally, ensuring mocks are isolated. (link)
| "builtins.open", mock.mock_open() | ||
| ) as mock_open: | ||
| with ( | ||
| mock.patch.object(os.path, "exists", return_value=True), |
There was a problem hiding this comment.
Patching os.path.exists globally via mock.patch.object violates the repository style guide's localized mocking rule. Please mock the local module import path google.auth.transport._mtls_helper.os.path.exists instead to keep the mock isolated.
| mock.patch.object(os.path, "exists", return_value=True), | |
| mock.patch("google.auth.transport._mtls_helper.os.path.exists", return_value=True), |
References
- Localized Mocking: When mocking standard functions or filesystem checks (like path.exists), mock the local module import path instead of patching builtins globally, ensuring mocks are isolated. (link)
| }, | ||
| ), | ||
| mock.patch("builtins.open", mock.mock_open(read_data=fake_config_content)), | ||
| mock.patch("os.path.exists", return_value=True), |
There was a problem hiding this comment.
Patching os.path.exists globally violates the repository style guide's localized mocking rule. Please mock the local module import path google.auth.transport._mtls_helper.path.exists instead to ensure mocks are isolated.
| mock.patch("os.path.exists", return_value=True), | |
| mock.patch("google.auth.transport._mtls_helper.path.exists", return_value=True), |
References
- Localized Mocking: When mocking standard functions or filesystem checks (like path.exists), mock the local module import path instead of patching builtins globally, ensuring mocks are isolated. (link)
| ( | ||
| reformatted_signed_request.get("headers").append( | ||
| {"key": "x-goog-cloud-target-resource", "value": AUDIENCE} | ||
| ), | ||
| ) |
There was a problem hiding this comment.
This parenthesized expression with a trailing comma creates an unused 1-tuple containing None (since list.append returns None). This is unnecessary and confusing. It should be simplified to a direct method call statement.
reformatted_signed_request.get("headers").append(
{"key": "x-goog-cloud-target-resource", "value": AUDIENCE}
)1 participant
As part of #18134, we identified a number of packages that are still using black for formatting, inconsistent with the rest of the repo. This PR updates them to use ruff, and re-formats the files