-
Notifications
You must be signed in to change notification settings - Fork 698
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
Make lookupStreamBySubject public #1114
Conversation
Signed-off-by: Jarema <[email protected]>
Signed-off-by: Jarema <[email protected]>
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 that the new API need to conform to others and accept []jSOpt. What do you think?
jsm.go
Outdated
@@ -1515,6 +1518,31 @@ func (jsc *js) StreamNames(opts ...JSOpt) <-chan string { | |||
return ch | |||
} | |||
|
|||
// StreamNameBySubject returns a stream name that matches the subject. | |||
func (jsc *js) StreamNameBySubject(subj string) (string, error) { |
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 am not sure this is right. Since this becomes a context level API, should you pass the js options as a vararg? Then inside you would call getJSContextOpts(), etc... User should be able to override the wait and apiSubj() still need fixing to make sure it uses any user domain provided override (granted not part of this PR).
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.
While I'm not a huge fan of the concept of having 1 common option type for all interface methods, in this case I agree. It makes sense to allow user to at least pass context/wait and be consistent with the rest of the interface at the same time.
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.
agree. Updated the signature, and used jsc.apiRequestWithContext
to at least use the context that can be passed, but still, it's misleading to give the user option to pass Domain
and ApiPrefix
and not use it.
That's consistent and I agree to have it, but it only adds up to what #1104 refers to.
Signed-off-by: Jarema <[email protected]>
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.
LGTM
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.
LGTM
Working on DAPR adapter for NATS (dapr/components-contrib#2161), I want to move from creating a Consumer by calling
js.Subscribe()
to explicitjs.AddConsumer
to avoid some errors.Was surprised that this method is not public.