Skip to content
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

[ADDED] Option setter for the URL client connects to #95

Merged
merged 2 commits into from
Jul 19, 2016

Conversation

kozlovic
Copy link
Member

Also fixed the pub and sub example to use this option setter. They
would otherwise only be able to connect to localhost:4222.

Resolves #94

Also fixed the pub and sub example to use this option setter. They
would otherwise only be able to connect to localhost:4222.

Resolves #94
@coveralls
Copy link

coveralls commented Jul 17, 2016

Coverage Status

Coverage increased (+0.1%) to 86.526% when pulling 3f726b0 on add_nats_url_option_setter into 1af8219 on master.

@kozlovic
Copy link
Member Author

@ColinSullivan1 @aricart Please review at your earliest convenience on Monday.
@mcqueary We may want to issue a new release after this PR is merged. There has been few other option setters added and fixes since v.0.1.0.

@kozlovic
Copy link
Member Author

@EduardFazliev No because we use . for the import, so we don't need to specify stan. Not sure why this example has been written this way as opposed to the pub one. Will update as part of a different PR.

var clusterID string
var clientID string
var async bool
URL := stan.DefaultNatsURL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this necessary with the flags below?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, will use var instead.

@ColinSullivan1
Copy link
Member

LGTM.

@@ -57,10 +55,12 @@ func main() {
var qgroup string
var unsubscribe bool

URL := DefaultNatsURL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above... Nitpicky, I know. ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will change it to var. Thanks!

@aricart
Copy link
Member

aricart commented Jul 19, 2016

LGTM!

@coveralls
Copy link

coveralls commented Jul 19, 2016

Coverage Status

Coverage increased (+0.1%) to 86.526% when pulling d6edc7b on add_nats_url_option_setter into 1af8219 on master.

@kozlovic kozlovic merged commit beba61d into master Jul 19, 2016
@kozlovic kozlovic deleted the add_nats_url_option_setter branch July 19, 2016 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem in connect to remote stan server
5 participants