-
Notifications
You must be signed in to change notification settings - Fork 477
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
Prefer importlib.metadata over pkg_resources if available #2081
Conversation
sentry_sdk/integrations/modules.py
Outdated
from importlib.metadata import distributions, version | ||
|
||
for dist in distributions(): | ||
yield dist.metadata["Name"], version(dist.metadata["Name"]) |
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's also dist.name
(it's just an alias of dist.metadata["Name"]
), but it hasn't always been there. dist.metadata["Name"]
should work with all py versions.
74f8a87
to
15893cf
Compare
15893cf
to
4347c32
Compare
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.
If distributions()
yields the same stuff as pkg_resources.working_set
this looks good and can be merged.
@antonpirker It does with one difference:
Do you see this creating issues anywhere? I grepped for where we actually use the modules and it seems like we attach them to the event and check them here but elsewhere we seem to be using |
@sentrivana could you please add a test for testing |
ccb7d14
to
86b6f7d
Compare
Co-authored-by: Antoni Szych <antoni.szych@rtbhouse.com>
@antonpirker Changed the PR a bit and added a test, could you please skim over to see if it still looks ok to you? |
sentry_sdk/integrations/modules.py
Outdated
@@ -18,15 +18,30 @@ | |||
_installed_modules = None | |||
|
|||
|
|||
def _normalize_module_name(name): | |||
# type: (str) -> str | |||
return name.lower().replace("-", "_") |
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.
Why do we need to replace the "-" with a "_"?
Wouldn't we then report a package name "sentry_sdk" that is different than the one the user installs "sentry-sdk"?
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's a weird mismatch between importlib
and pkg_resources
where they report the package typing-extensions
differently; importlib
reports typing_extensions
, while pkg_resources
reports it as typing-extensions
. This is the only package that is different (if we normalize to lowercase, which we're doing). I'll remove the .replace()
and change the test to ignore the one package.
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.
I guess if we just try to keep as much the same as it was reported with pkg_resources
we are good to go.
Let's not use
pkg_resources
and avoid aDeprecationWarning
ifimportlib.metadata
is available.Fixes #2048