-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add notifications api #531
Conversation
This is not fully ready yet, but most of the implementation is done, still need to write all the tests and make sure this fully works, but would be nice to get early feedback. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #531 +/- ##
==========================================
- Coverage 56.29% 54.94% -1.35%
==========================================
Files 122 129 +7
Lines 13364 13906 +542
==========================================
+ Hits 7523 7641 +118
- Misses 5389 5809 +420
- Partials 452 456 +4 ☔ View full report in Codecov by Sentry. |
This looks great! One thing that I'm not sure about atm is how many times we should (re)attempt an enqueue for a notify message? Our default logic is to retry until the timeout time (which is set to the same time as the promise timeout), does this make sense for notify? It's certainly the easiest to implement because we can just leave the logic as is. |
Our other option is to just try to send it and do not care about any errors. I think the spirit of notify goes more in that direction.
… On Jan 15, 2025, at 1:28 PM, David Farr ***@***.***> wrote:
This looks great!
One thing that I'm not sure about atm is how many times we should (re)attempt an enqueue for a notify message? Our default logic is to retry until the timeout time (which is set to the same time as the promise timeout), does this make sense for notify? It's certainly the easiest to implement because we can just leave the logic as is.
—
Reply to this email directly, view it on GitHub <#531 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACVCDLLDI24I3IDYHZ534PT2K3HJVAVCNFSM6AAAAABVH5YAVKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKOJTHE3TGNBQG4>.
You are receiving this because you authored the thread.
|
I'm up for that!
On Wed, Jan 15, 2025 at 1:32 PM Andres Villegas ***@***.***>
wrote:
… Our other option is to just try to send it and do not care about any
errors. I think the spirit of notify goes more in that direction.
> On Jan 15, 2025, at 1:28 PM, David Farr ***@***.***> wrote:
>
>
> This looks great!
>
> One thing that I'm not sure about atm is how many times we should
(re)attempt an enqueue for a notify message? Our default logic is to retry
until the timeout time (which is set to the same time as the promise
timeout), does this make sense for notify? It's certainly the easiest to
implement because we can just leave the logic as is.
>
> —
> Reply to this email directly, view it on GitHub <
#531 (comment)>,
or unsubscribe <
https://github.com/notifications/unsubscribe-auth/ACVCDLLDI24I3IDYHZ534PT2K3HJVAVCNFSM6AAAAABVH5YAVKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKOJTHE3TGNBQG4>.
> You are receiving this because you authored the thread.
>
—
Reply to this email directly, view it on GitHub
<#531 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKS2OSUXQ7PGBPXD2UJX5L2K3HVHAVCNFSM6AAAAABVH5YAVKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKOJTHE3TQNRWG4>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
b7532e5
to
39a9ce1
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.
Damn man, even the cli is here!
Co-authored-by: David Farr <[email protected]>
No description provided.