-
Notifications
You must be signed in to change notification settings - Fork 451
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
Statically typed notifier #6728
Statically typed notifier #6728
Conversation
68666cb
to
6cc48d9
Compare
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 work! Overall, the syntax to invoke a particular notification topic looks like this:
self.notifier[notifications.channel_discovered]({"results": results, "uuid": str(CHANNELS_VIEW_UUID)})
I like the syntax - I can see that it's less error prone and it's intuitive to use.
At the same time, looking at the old notifier again, I believe our old notifier functioned "just good enough" and I personally have not used the notifier in a very long time (maybe because I mainly focus on the GUI parts so other devs might disagree there). As such, the long-term benefit of this refactoring effort is, at least for me, unclear.
Nonetheless, I appreciate the contribution and since the PR seems to be nearly complete, I would be in favour of finishing it up and merging it 👍
841625e
to
544b980
Compare
retest this please |
456308a
to
3197f7d
Compare
3197f7d
to
107e026
Compare
@devos50, thank you for the review! I simplified the logic of the Notifier and removed unnecessary methods. Also, I added the A statically typed notifier should prevent bugs like #6753 fixes: all parameters to I also added the documentation to the Notifier:
|
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 processing my comments! Note that I didn't do a full thorough review again (since the PR is quite big and I already looked at it earlier) but my concerns are addressed. I recommend asking another Tribler dev for a second review before merging (I think @xoriole is already requested?) 👍
395a101
to
3565b57
Compare
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.
Nice 👍
No description provided.