-
Notifications
You must be signed in to change notification settings - Fork 8
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
Bandit, release v1, use ifs over assert in src #67
base: master
Are you sure you want to change the base?
Conversation
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
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.
Excellent! 👍
@@ -4,7 +4,7 @@ | |||
|
|||
""" | |||
|
|||
__version__ = "0.5.0" | |||
__version__ = "1.0.0" |
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.
Down with ZeroVer! 💯
(Nothing to do here, just pleased with this 😋)
(timeout is not None or max_tries is not None) and poll_forever | ||
), "You cannot specify both the option to poll_forever and max_tries/timeout." | ||
if not ((timeout is not None or max_tries is not None) or poll_forever): | ||
raise AssertionError( |
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.
Since we're including this in a major release, I don't think it would be unreasonable to break compatibility by switching to a ValueError
.
(FWIW, I don't actually need/rely on this at all. It's just more correct IMO.)
Alternatively, a compromise solution could be to create a custom exception for a while that inherits from both:
class ValueAssertionError(ValueError, AssertionError):
"""Backwards-compatible ValueError (catching AssertionError is deprecated!)"""
Fix for this issue #48
Also use bandit in pre-commit.