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

Fixing deprecated version attribute #2338

Merged
merged 11 commits into from
Sep 6, 2023

Conversation

vagi8
Copy link
Contributor

@vagi8 vagi8 commented Sep 1, 2023

Creating a fix for #2316

@sentrivana
Copy link
Contributor

Heya @vagi8, thanks for the contribution! This will work, but not in Python versions lower than 3.8 since importlib.metadata was only added in 3.8 -- and we still want to support those.

The good news is, there is already a place in the SDK where we gather all installed packages and their versions. Have a look at this:

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

Copy link
Contributor

@sentrivana sentrivana left a comment

Choose a reason for hiding this comment

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

See my previous comment. :)

@vagi8
Copy link
Contributor Author

vagi8 commented Sep 1, 2023

Hey @sentrivana
Thanks for bringing this to my attention.
Possible solution I can think of

  1. use importlib.metadata and pkg_resources along with a try except block inside the flask.py to get the version. (pkg_resources.get_distribution("flask").version). Similar to the '_generate_installed_modules' function but specifically for flask.
  2. Reuse the _get_installed_modules as is and extract flask version. also rename the function without the leading underscore
  3. Re-design the '_get_installed_modules' to also fetch the version for a specific module if given an input.

@sentrivana
Copy link
Contributor

Hey @vagi8. I'd go with option 2. The _get_installed_modules function is set up so that the actual module version collection only runs once the first time it's called and is then cached in a global variable. Any subsequent calls to it will just read the global variable. So calling it from the Flask integration wouldn't mean that it would be gathering info for all modules again, if that's your concern. It's fine to call it as is.

Let's please keep the leading underscore though, the intention there is to mark it as not part of the public API.

@vagi8 vagi8 closed this Sep 4, 2023
@vagi8 vagi8 reopened this Sep 4, 2023
@@ -44,6 +44,8 @@
except ImportError:
raise DidNotEnable("blinker is not installed")

installed_packages = _get_installed_modules()
FLASK_VERSION = installed_packages("flask")
Copy link
Contributor

Choose a reason for hiding this comment

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

_get_installed_modules() returns a dict, so you'd want:

Suggested change
FLASK_VERSION = installed_packages("flask")
FLASK_VERSION = installed_packages["flask"]

I'm also thinking we could move these two lines into the setup_once method instead, so that all the version checking code is in one place.

@@ -44,6 +44,7 @@
except ImportError:
raise DidNotEnable("blinker is not installed")

FLASK_VERSION = None
Copy link
Contributor

@sentrivana sentrivana Sep 5, 2023

Choose a reason for hiding this comment

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

Is this global variable needed? We only seem to be using FLASK_VERSION inside the setup_once function, so we just define and use it there?

Copy link
Contributor

@sentrivana sentrivana left a comment

Choose a reason for hiding this comment

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

Looking great @vagi8, thank you for the contribution! 🚀

One last thing: flake8 is complaining about the variable name now: sentry_sdk/integrations/flask.py:69:10: N806 variable 'FLASK_VERSION' in function should be lowercase, once we fix that we're good to merge 👍🏻

@sentrivana sentrivana enabled auto-merge (squash) September 6, 2023 08:03
auto-merge was automatically disabled September 6, 2023 08:28

Head branch was pushed to by a user without write access

@sentrivana sentrivana enabled auto-merge (squash) September 6, 2023 08:55
@sentrivana sentrivana merged commit 0fb0dea into getsentry:master Sep 6, 2023
sentrivana added a commit that referenced this pull request Sep 18, 2023

---------

Co-authored-by: Ivana Kellyerova <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants