CyCoreSystems / ari

Golang Asterisk REST Interface (ARI) library
Apache License 2.0
189 stars 73 forks source link

Update example base on the new implementation of ari.Client #71

Closed baldrailers closed 7 years ago

baldrailers commented 7 years ago

This change is Reviewable

Ulexus commented 7 years ago

Thanks for the very quick update. I completely forgot about the examples.

I knew I should have changed the name of native.New(). That now really just creates a new Client, without actually connecting (the websocket) to anything.

This example may actually work, because it doesn't rely on any ARI application (triggered via websocket) being active, I'd prefer to use the new native.Connect() in examples, since that should be what most users use. It combines the creation of the client and its connection.


Reviewed 1 of 1 files at r1. Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

baldrailers commented 7 years ago

I was just getting started exploring the library. I should check native.Connect() instead. Thanks for the feedback.


Comments from Reviewable

Ulexus commented 7 years ago

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


_examples/helloworld/main.go, line 34 at r1 (raw file):

  log.Info("Connecting")

  cl := native.New(&opts)

For documentary purposes, let's use native.Connect(nil) here.


Comments from Reviewable

baldrailers commented 7 years ago

_examples/helloworld/main.go, line 34 at r1 (raw file):

Previously, Ulexus (Seán C. McCord) wrote…
For documentary purposes, let's use `native.Connect(nil)` here.

native.Connect() is expecting some context, if I pass native.Connect(nil, &opts) this will throw out Segmentation Faults. I wonder what is expected to be in that ctx parameter.


Comments from Reviewable

Ulexus commented 7 years ago

Review status: all files reviewed at latest revision, 1 unresolved discussion.


_examples/helloworld/main.go, line 34 at r1 (raw file):

Previously, baldrailers (JAF) wrote…
`native.Connect()` is expecting some `context`, if I pass `native.Connect(nil, &opts)` this will throw out Segmentation Faults. I wonder what is expected to be in that `ctx` parameter.

Yes, the new version of the ARI client uses the context for lifecycle control. When/if that context is closed, the client will be closed, as well. For this example, you can use context.TODO()(context here is the Go 1.7-introduced context package; or for earlier versions of Go, the x/net/context)


Comments from Reviewable

Ulexus commented 7 years ago

Review status: all files reviewed at latest revision, 1 unresolved discussion.


_examples/helloworld/main.go, line 34 at r1 (raw file):

Previously, Ulexus (Seán C. McCord) wrote…
Yes, the new version of the ARI client uses the context for lifecycle control. When/if that context is closed, the client will be closed, as well. For this example, you can use `context.TODO()`(context here is the Go 1.7-introduced `context` package; or for earlier versions of Go, the `x/net/context`)

Looking at it, though, I'm not sure why we are exposing it here. The native client doesn't actually use it for anything other than lifecycle control, and it has a perfectly good Close() function. I'll rewrite this to not require a context to be passed in. It seems rather silly.


Comments from Reviewable

Ulexus commented 7 years ago

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


_examples/helloworld/main.go, line 34 at r1 (raw file):

Previously, Ulexus (Seán C. McCord) wrote…
Looking at it, though, I'm not sure why we are exposing it here. The native client doesn't actually use it for anything _other_ than lifecycle control, and it has a perfectly good `Close()` function. I'll rewrite this to not require a context to be passed in. It seems rather silly.

Okay, Connect() no longer expects a context to be passed.


Comments from Reviewable

baldrailers commented 7 years ago

_examples/helloworld/main.go, line 34 at r1 (raw file):

Previously, Ulexus (Seán C. McCord) wrote…
Okay, `Connect()` no longer expects a context to be passed.

But I just made some updates base on your recommendations ;)


Comments from Reviewable

Ulexus commented 7 years ago

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


_examples/helloworld/main.go, line 34 at r1 (raw file):

Previously, baldrailers (JAF) wrote…
But I just made some updates base on your recommendations ;)

Ah, I'm sorry. I just saw that, and it didn't make any sense to me why we were requesting a context to be passed in.


Comments from Reviewable

Ulexus commented 7 years ago

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


_examples/helloworld/main.go, line 34 at r1 (raw file):

Previously, Ulexus (Seán C. McCord) wrote…
Ah, I'm sorry. I just saw that, and it didn't make any sense to me why we were requesting a context to be passed in.

I'm putting the keyboard down, now...


Comments from Reviewable

baldrailers commented 7 years ago

_examples/helloworld/main.go, line 34 at r1 (raw file):

Previously, Ulexus (Seán C. McCord) wrote…
I'm putting the keyboard down, now...

Yeah no worries, I will just watch the upstream and update the example accordingly. You may stumble into something even deeper when you removed the context.


Comments from Reviewable

Ulexus commented 7 years ago

Reviewed 1 of 1 files at r2. Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

Ulexus commented 7 years ago

Thanks for the contribution!