-
Notifications
You must be signed in to change notification settings - Fork 553
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
Fix repeated alerts by using type instead of instanceof #608
Fix repeated alerts by using type instead of instanceof #608
Conversation
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.
Thanks for your contribution, and good catch on the bug 🙏 One nitpick and then I think we can include this in the 1.9.0 release today or tomorrow.
More generally - the alert
functionality was implemented quite a while ago. If you have any feedback or ideas for how it can be improved based on your use case, I'd love to hear. I'm available on Gitter or Telegram with the same handle as I use on Github.
brownie/network/alert.py
Outdated
@@ -90,7 +90,7 @@ def _loop( | |||
start_value = value | |||
if not repeat: | |||
repeat = None | |||
elif isinstance(repeat, int): | |||
elif type(repeat) == int: |
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.
It's more verbose, but I think preferable to use isinstance(repeat, int) and not isinstance(repeat, bool)
- because Wei
is an int
subclass and commonly used for return values, I can imagine a situation where the user is trying to set an alert with that type.
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.
Makes sense. Updated to reflect this.
I'll keep a lookout for possible improvements while we apply our usecase to it 👍
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.
Thanks!
What I did
Fix repeated alerts
How I did it
Used
type
instead ofinstanceof
, becausebool
is a subclass ofint
in python.How to verify it
It's a small change. Also automated tests should pickup if this is broken.
Checklist