add connection open properties #31
Conversation
devigned
force-pushed
the
feature/conn-props
branch
from
8428601
to
81dbc5c
Compare
conn.go
Outdated
| c.properties = make(map[symbol]interface{}) | ||
| } | ||
| sym := symbol(key) | ||
| c.properties[sym] = value |
There was a problem hiding this comment.
Simplify to c.properties[symbol(key)] = value.
conn.go
Outdated
| @@ -128,6 +128,18 @@ func ConnMaxSessions(n int) ConnOption { | |||
| } | |||
| } | |||
|
|
|||
| // ConnProperty sets an entry in the connection properties | |||
There was a problem hiding this comment.
// ConnProperty sets an entry in the connection properties map sent to the server.
//
// This option can be used multiple times.
conn.go
Outdated
| @@ -198,6 +211,7 @@ func newConn(netConn net.Conn, opts ...ConnOption) (*conn, error) { | |||
| delSession: make(chan *Session), | |||
| txFrame: make(chan frame), | |||
| txDone: make(chan struct{}), | |||
| properties: make(map[symbol]interface{}), | |||
There was a problem hiding this comment.
We want properties to be nil when none are specified, so this line is unnecessary.
conn_test.go
Outdated
| if c.properties[symbol(key)] != value { | ||
| t.Errorf("conn property was not set to key \"%s\" and value \"%s\"", key, value) | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not a big fan of tests that do something relatively simple and then check that it did the thing.
I'd rewrite this as a table test ensuring different sets of options produce the correct results. I'll create an issue to track adding that sort of test unless you'd particularly like to take that on in this PR (there's no expectation on my part that you do).
integration_test.go
Outdated
| @@ -18,7 +18,7 @@ import ( | |||
| "testing" | |||
| "time" | |||
|
|
|||
| "github.com/Azure/azure-sdk-for-go/arm/servicebus" | |||
| "github.com/Azure/azure-sdk-for-go/services/servicebus/mgmt/2017-04-01/servicebus" | |||
conn_test.go
Outdated
| } | ||
|
|
||
| if c.properties[symbol(key)] != value { | ||
| t.Errorf("conn property was not set to key \"%s\" and value \"%s\"", key, value) |
There was a problem hiding this comment.
Just FYI, the %q formatter is handy when you want to quote strings. It's particularly useful when comparing strings that may have unprintable characters. https://play.golang.org/p/xx2QI3_4Irr
There was a problem hiding this comment.
Oh, that's fancy. Thanks for spreading the knowledge.
conn_test.go
Outdated
| value := "propValue" | ||
| err := ConnProperty(key, value)(c) | ||
| if err != nil { | ||
| t.Error(fmt.Sprintf("%+v", err)) |
There was a problem hiding this comment.
I don't know how this sort of thing ended up in the other tests, but it should just be t.Errorf("%+v", err). I need to correct my usages.
… error. * Adds exported `ErrConnClosed`, `ErrSessionClosed`, and `ErrLinkClosed` indicating a library consumer requested close. * Some refactoring to conn close logic to remove race condition where another component could acquire conn.errMu after conn.mux releases it but before conn.close acquires it. * Copied integration test fixes from #31. Fixes #33
… error. (#34) * Adds exported `ErrConnClosed`, `ErrSessionClosed`, and `ErrLinkClosed` indicating a library consumer requested close. * Some refactoring to conn close logic to remove race condition where another component could acquire conn.errMu after conn.mux releases it but before conn.close acquires it. * Copied integration test fixes from #31. Fixes #33
devigned
force-pushed
the
feature/conn-props
branch
from
81dbc5c
to
b1ef7ce
Compare
|
@vcabbage I added an error check for empty keys. I don't think an empty symbol makes sense. |
|
Thanks! |
|
My pleasure. Thank you for the feedback and the quick merge. |
Resolves: #28