Skip to content
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

feat: add ability to remove unversioned modules #2329

Merged
merged 13 commits into from
Feb 14, 2025

Conversation

parthea
Copy link
Contributor

@parthea parthea commented Feb 12, 2025

Googlers see go/gapic-default-imports

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Feb 12, 2025
@parthea parthea marked this pull request as ready for review February 12, 2025 23:20
@parthea parthea requested a review from a team as a code owner February 12, 2025 23:20
@parthea parthea requested review from vchudnov-g and ohmayr February 12, 2025 23:22
@@ -145,6 +146,50 @@ def test_get_response_ignore_gapic_metadata():
assert res.file == CodeGeneratorResponse().file


@pytest.mark.parametrize("unversioned_package_disabled", (True, False))
def test_get_response_ignore_unversioned_package(unversioned_package_disabled):
g = make_generator()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use one character variable names. I see the same construct is used in other tests, but I advocate not perpetuating the practice.

Suggested change
g = make_generator()
generator = make_generator()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 20f4050

service_yaml_config=service_config,
)

with mock.patch.object(jinja2.FileSystemLoader, "list_templates") as lt:
Copy link
Contributor

@vchudnov-g vchudnov-g Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still suggest avoiding short names if possible, though these are two-character and used closed to the definition, which makes it more amenabel IMO.

Consider s/lt/lister/ and s/gt/getter/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6491e53

@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Feb 14, 2025
@parthea parthea enabled auto-merge (squash) February 14, 2025 00:10
@parthea parthea merged commit ccd619f into main Feb 14, 2025
126 checks passed
@parthea parthea deleted the allow-disabling-unversioned-packages branch February 14, 2025 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants