Initial support for IRCv3 CAP negotiation

[belak]

This is related to #53

Based on http://ircv3.net/specs/core/capability-negotiation-3.1.html and how some odd servers respond to CAP ACK requests.

TODO:

• Clean up code - specifically cyclo complexity of handleCapHandshake
• Answer questions leftover in Support CAP negotiation in the Client #53 - Current plan is "yes" so we can re-use the same event-loop.
• Remove the debug c.CapRequest calls
• Remove any debug printf calls
• Tests
• Docs
• Improve/fix error handling when an error is sent in a handler
• Handle multi-line CAP LS response This is a 3.2 extension which I'm holding off on for now

[jsvana]

overall looks good but some questions

[jsvana]

this and Enabled seem like states for this. should they be one field with an enum?

[belak]

I've experimented a bit with it and ended up with this:

diff --git a/client.go b/client.go
index 359a89a..64f47c1 100644
--- a/client.go
+++ b/client.go
@@ -69,7 +69,7 @@ var clientFilters = map[string]func(*Client, *Message){
 		case "ACK":
 			for _, key := range strings.Split(m.Trailing(), " ") {
 				cap := c.caps[key]
-				cap.Enabled = true
+				cap.State = capStateEnabled
 				c.caps[key] = cap
 			}
 			c.remainingCapResponses--
@@ -77,8 +77,8 @@ var clientFilters = map[string]func(*Client, *Message){
 			// If we got a NAK and this REQ was required, we need to bail
 			// with an error.
 			for _, key := range strings.Split(m.Trailing(), " ") {
-				if c.caps[key].Required {
-					c.sendError(fmt.Errorf("CAP %s requested but was rejected", key))
+				if c.caps[key].State == capStateRequired {
+					c.sendError(fmt.Errorf("CAP %s required but was rejected", key))
 					return
 				}
 			}
@@ -87,8 +87,8 @@ var clientFilters = map[string]func(*Client, *Message){
 
 		if c.remainingCapResponses <= 0 {
 			for key, cap := range c.caps {
-				if cap.Required && !cap.Enabled {
-					c.sendError(fmt.Errorf("CAP %s requested but not accepted", key))
+				if cap.State == capStateRequired {
+					c.sendError(fmt.Errorf("CAP %s required but not accepted", key))
 					return
 				}
 			}
@@ -121,15 +121,21 @@ type ClientConfig struct {
 	Handler Handler
 }
 
-type cap struct {
-	// Requested means that this cap was requested by the user
-	Requested bool
+type capState int
 
-	// Required will be true if this cap is non-optional
-	Required bool
+const (
+	capStateNone capState = iota
+	// This cap was requested by the user
+	capStateRequested
+	// This cap was requested and is non-optional
+	capStateRequired
+	// This cap was accepted by the server
+	capStateEnabled
+)
 
-	// Enabled means that this cap was accepted by the server
-	Enabled bool
+type cap struct {
+	// State includes whether or not the cap is Requested, Required, or Enabled.
+	State capState
 
 	// Available means that the server supports this cap
 	Available bool
@@ -288,7 +294,7 @@ func (c *Client) maybeStartCapHandshake() error {
 	c.Write("CAP LS")
 	c.remainingCapResponses = 1 // We count the CAP LS response as a normal response
 	for key, cap := range c.caps {
-		if cap.Requested {
+		if cap.State == capStateRequested || cap.State == capStateRequired {
 			c.Writef("CAP REQ :%s", key)
 			c.remainingCapResponses++
 		}
@@ -304,8 +310,13 @@ func (c *Client) maybeStartCapHandshake() error {
 // negotiated during the handshake.
 func (c *Client) CapRequest(capName string, required bool) {
 	cap := c.caps[capName]
-	cap.Requested = true
-	cap.Required = cap.Required || required
+	if cap.State != capStateRequired {
+		if required {
+			cap.State = capStateRequired
+		} else {
+			cap.State = capStateRequested
+		}
+	}
 	c.caps[capName] = cap
 }
 
@@ -313,7 +324,7 @@ func (c *Client) CapRequest(capName string, required bool) {
 // that it will not be populated until after the CAP handshake is done, so it is
 // recommended to wait to check this until after a message like 001.
 func (c *Client) CapEnabled(capName string) bool {
-	return c.caps[capName].Enabled
+	return c.caps[capName].State == capStateEnabled
 }
 
 // CapAvailable allows you to check if a CAP is available on this server. Note
diff --git a/client_test.go b/client_test.go
index 2ed3c96..1252412 100644
--- a/client_test.go
+++ b/client_test.go
@@ -126,7 +126,7 @@ func TestCapReq(t *testing.T) {
 	assert.False(t, c.CapAvailable("random-thing"))
 	assert.True(t, c.CapAvailable("multi-prefix"))
 
-	c = runClientTest(t, config, errors.New("CAP multi-prefix requested but was rejected"), func(c *Client) {
+	c = runClientTest(t, config, errors.New("CAP multi-prefix required but was rejected"), func(c *Client) {
 		assert.False(t, c.CapAvailable("random-thing"))
 		assert.False(t, c.CapAvailable("multi-prefix"))
 		c.CapRequest("multi-prefix", true)
@@ -144,7 +144,7 @@ func TestCapReq(t *testing.T) {
 	assert.False(t, c.CapAvailable("random-thing"))
 	assert.True(t, c.CapAvailable("multi-prefix"))
 
-	c = runClientTest(t, config, errors.New("CAP multi-prefix requested but not accepted"), func(c *Client) {
+	c = runClientTest(t, config, errors.New("CAP multi-prefix required but not accepted"), func(c *Client) {
 		assert.False(t, c.CapAvailable("random-thing"))
 		assert.False(t, c.CapAvailable("multi-prefix"))
 		c.CapRequest("multi-prefix", true)

I think it's a little bit more fragile, so I'd prefer to keep the current implementation.

[jsvana]

should you log if cap.Required is false and you're explicitly marking it as true?

[belak]

Probably not. If one plugin requests it as optional and one plugin requests it as required, it should be marked as required. Plus the logging in go-irc is somewhat limited on purpose.

[jsvana]

worthwhile to not kill caller if you request an unknown cap?

[belak]

It won't. Because of how go works, c.caps[capName] will return the zero value for a cap which will be False for all values.

This is actually handled during the tests. :)

[jsvana]

doc for this? also seems like an odd name. maybe add logging here when it doesn't start the handshake

[belak]

Yeah, I'll add docs for all the maybeStartX methods.

[belak]

I'm merging this, but I'll avoid tagging it for a bit, until I can be sure it's at least reasonably stable.