-
Notifications
You must be signed in to change notification settings - Fork 572
fix(threading): Handle channels shadowing #5299
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
alexander-alderman-webb
left a comment
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.
💯
|
|
||
| channels_version = channels.__version__ | ||
| except ImportError: | ||
| except (ImportError, AttributeError): |
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 implies that if there is an AttributeError because of channels line, django_version will be set to None and that is not ideal... perhaps we should split this try/except in 2 parts, 1 for django and another for channels?
|
|
||
| channels_version = channels.__version__ | ||
| except ImportError: | ||
| except (ImportError, AttributeError): |
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 implies that if there is an AttributeError because of channels line, django_version will be set to None and that is not ideal... perhaps we should split this try/except in 2 parts, 1 for django and another for channels?
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.
That's ok, django_version and channels_version both are only used for warning about a very specific Django/channels/sentry-sdk incompatibility that only appears in specific versions.
But you raise a good point in that it's not clear what the variable is used for. It might happen that someone extends the logic at some point using django_version for some other purpose, not realizing it's not always populated correctly. So it's worth making this nicer.
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.
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. Bug Fixes 🐛
Documentation 📚
Internal Changes 🔧
🤖 This preview updates automatically when you update the PR. |
Description
Folks might have a
channelsmodule that has nothing to do with thechannelspackage we're expecting. This will then make the code throw anAttributeErrorwhich we don't handle.Issues
Reminders
tox -e linters.feat:,fix:,ref:,meta:)