GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Fixing deprecated version attribute by vagi8 · Pull Request #2338 · getsentry/sentry-python Conversation
Contributor
Creating a fix for #2316
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Collaborator
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
sentrivana
requested changes
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my previous comment. :)
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
|
There was an error while loading. Please reload this page.
|
Hey @sentrivana
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Hey @vagi8. I'd go with option 2. The Let's please keep the leading underscore though, the intention there is to mark it as not part of the public API. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| raise DidNotEnable("blinker is not installed") | ||
|
|
||
| installed_packages = _get_installed_modules() | ||
| FLASK_VERSION = installed_packages("flask") |
There was a problem 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:
| 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…try-python into deprecate-version-attr
| except ImportError: | ||
| raise DidNotEnable("blinker is not installed") | ||
|
|
||
| FLASK_VERSION = None |
There was an error while loading. Please reload this page.
There was a problem 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?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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 👍🏻
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Head branch was pushed to by a user without write access
Co-authored-by: Ivana Kellyerova <[email protected]>
--------- Co-authored-by: Ivana Kellyerova <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.