-
Notifications
You must be signed in to change notification settings - Fork 73
fix: drop packaging and pkg_resources #2477
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
Conversation
|
|
||
| ParsedVersion = Tuple[int, ...] | ||
|
|
||
| def parse_version_to_tuple(version_string: str) -> ParsedVersion: # pragma: NO COVER |
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 is duplicated from https://github.com/googleapis/python-api-core/blob/main/google/api_core/_python_package_support.py. At the very least, add a TODO to remove this once the GAPICs can depend on an appropriate version of api_core, but ideally you can include this in a conditional to define this only if this symbol does not exist in the imported api_core.
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.
Hmm good point, but I noticed it's not technically a public method in api_core. So I'm not sure if we really should be referencing it in here if we have to grab it from the internal google.api_core._python_package_support module. In hindsight, this probably should have been exported in the __init__ with the others.
But I also notice that this block of code could be moved within this conditional, which is already commented in the way you describe. Would that be sufficient?
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.
@daniel-sanche Your suggestion sounds good to me. Good catch!
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.
Yes, putting it in the conditional is what I had in mind.
And you're right that we should export the symbol in the __init__. I'll do that.
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.
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.
Ok, I made the change, let me know if that works
Similar to googleapis/python-api-core#852
Fixes #2475