-
Notifications
You must be signed in to change notification settings - Fork 55
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 JetStream NATS Client Extensions #598
Changes from 9 commits
fb1f98c
0b26f93
93405e2
6971b20
1b9dde4
f76397d
080bc93
5789094
f7c1d51
9d6ff1f
854b413
5b32c3e
dc5668a
0308bd3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
namespace NATS.Client.Core; | ||
|
||
public interface INatsSubscriptionManager | ||
{ | ||
public ValueTask RemoveAsync(NatsSubBase sub); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is public now should we XMLDoc it? (If 'public but really internal' perhaps an XMLDoc stating it is an internal API subject to change?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ public InboxSub( | |
string subject, | ||
NatsSubOpts? opts, | ||
NatsConnection connection, | ||
ISubscriptionManager manager) | ||
INatsSubscriptionManager manager) | ||
: base(connection, manager, subject, queueGroup: default, opts) | ||
{ | ||
_inbox = inbox; | ||
|
@@ -35,7 +35,7 @@ protected override void TryComplete() | |
} | ||
} | ||
|
||
internal class InboxSubBuilder : ISubscriptionManager | ||
internal class InboxSubBuilder : INatsSubscriptionManager | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Almost left a comment here but see that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. true. I guess it will break binary compatibility but we're coming out with a few breaking changes here applications will need to rebuild anyway. |
||
{ | ||
private readonly ILogger<InboxSubBuilder> _logger; | ||
#if NETSTANDARD2_0 | ||
|
@@ -46,7 +46,7 @@ internal class InboxSubBuilder : ISubscriptionManager | |
|
||
public InboxSubBuilder(ILogger<InboxSubBuilder> logger) => _logger = logger; | ||
|
||
public InboxSub Build(string subject, NatsSubOpts? opts, NatsConnection connection, ISubscriptionManager manager) | ||
public InboxSub Build(string subject, NatsSubOpts? opts, NatsConnection connection, INatsSubscriptionManager manager) | ||
{ | ||
return new InboxSub(this, subject, opts, connection, manager); | ||
} | ||
|
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.
If I had one 'thought' here...
maybe as
<remarks>
XmlDoc... explaining to people why this -isn't- a valuetask... e.x.<remarks>This method is expected to complete quickly to avoid further delays in processing; if complex work is required, it is recommended to offload to a channel or other out-of-band processor</remarks>
but... as usual may be overthinking >_<
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.
done
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.
I think it is an important detail to mention