Conversation
slingamn
left a comment
slingamn
left a comment
There was a problem hiding this comment.
Awesome stuff!
Could you merge this: DanielOaks/irctest#26
and then update the integration tests to comply with the new spec? Or if you merge that PR, I could do it myself.
| // sendSuccessfulSaslAuth means that a SASL auth attempt completed successfully, and is used to dispatch messages. | ||
| func sendSuccessfulSaslAuth(client *Client, rb *ResponseBuffer, forNS bool) { | ||
| // sendSuccessfulAccountAuth means that an account auth attempt completed successfully, and is used to dispatch messages. | ||
| func sendSuccessfulAccountAuth(client *Client, rb *ResponseBuffer, forNS, forSASL bool) { |
There was a problem hiding this comment.
forNS and forSASL are an XOR, right? Should this just be a single bool?
There was a problem hiding this comment.
Mmm. I have thoughts about this. forSASL is sort of a dodgy name, it's basically trying to capture the case of 'for login' vs 'for ... um, not logging in, like registration?' since those are the two places we use that function. forSASL's just the easiest way to present it, so long as we assume that NS identify is also 'for sasl'.
|
|
||
| if loginSuccessful { | ||
| sendSuccessfulSaslAuth(client, rb, true) | ||
| sendSuccessfulAccountAuth(client, rb, true, true) |
There was a problem hiding this comment.
true, false? (Or should we reduce this to a single bool?)
|
Good to merge! I filed DanielOaks/irctest#28 for the test updates; you can assign it to me, or take it yourself. |
|
Whoo, awesome! Thanks for the review mate, much appreciated |
2 participants
Basically, I pretty much rewrote the original spec which defines the
ACCcommand (update's over here). This changes things around so that it takes the right kind of replies, and sends the right kind of replies as well.I'm gonna update the help text and do some more testing on this.