Move installed modules code to utils #2429
Conversation
Pull Request is not mergeable
| def _get_installed_modules(): | ||
| # type: () -> Dict[str, str] | ||
| global _installed_modules | ||
| if _installed_modules is None: | ||
| _installed_modules = dict(_generate_installed_modules()) | ||
| return _installed_modules |
There was a problem hiding this comment.
Would it perhaps be a good idea to place this code, along with the _installed_modules into a static class, so we can avoid having a global variable? I guess the static class would still effectively be storing a global state, so perhaps it makes only a small difference here
There was a problem hiding this comment.
We could wrap this in a static class but then it'd be a bit inconsistent to have some installed modules related code in the class (the _get_installed_modules function above + the _installed_modules var) and some of it outside (e.g. the package_version helper). We could put everything in the class, but it's not great API to have to then import the class to use a small utility function. Also, you technically are introducing a global variable in any case, it's just about whether turning the whole thing into a class brings any additional benefits. I'd prefer to keep this as is if that's ok with you.
| @@ -488,3 +500,60 @@ def test_get_error_message(error, expected_result): | |||
| exc_value.detail = error | |||
| raise Exception | |||
| assert get_error_message(exc_value) == expected_result(exc_value) | |||
|
|
|||
|
|
|||
| def test_installed_modules(): | |||
There was a problem hiding this comment.
It looks to me as though this test is simply generating the installed module information in the same or a similar way that the actual code does it in utils.py. I would prefer that we somehow compare against a manually generated output, though, since this test will fail to detect a case where something changes in the importlib or pkg_resources modules that causes the test and the actual code to break in the same way, and it is also more difficult by reading this test case to determine what the intended output is.
Perhaps we can somehow mock out the installed modules so that we can manually generate the expected output, but admittedly doing this may be somewhat complicated. I also don't mind if we delete this test case, or replace it with a test case that simply calls the function and ensures we don't get an error, since I am unsure that it adds much value since we are just repeating the code from the function we are testing.
There was a problem hiding this comment.
So this test is a bit unconventional. On py versions where only one of importlib.metadata, pkg_resources exists, it's exactly as you write -- it isn't testing much. It only makes sense on py versions that have both importlib.metadata and pkg_resources, since in that case it compares whether our ways of getting and normalizing the package name yield the same result for packages from both importlib.metadata and pkg_resources.
...and introduce a
package_versionhelper function to use in integrations.Even though we're now using the
_get_installed_modulesfunction in many different places, it still lives insentry_sdk.integrations.modules. This PR:_get_installed_modules(and related helpers) toutils.pypackage_versionhelper function (also inutils.py) that finds out and parses the version of a package in one go.