support server password

[quite]

I'd like to set the server password. fluffle/goirc supports it, but your NewBot* functions does not. Would you be up for adding (having that added)? It'd break the API as it stands...

Btw I'm using goircbot for a git* webhook receiving bot; https://github.com/quite/pjodd But perhaps I'm slowly growing out of goircbot... thinking of multi-network support currently :)

[StalkR]

Hey, cool project :)

Indeed I didn't need IRC passwords so the API doesn't support it, and it would break to add it, but it was the same to add proxy support, and we added NewBotWithProxy.

Until we break everything to properly add an API with flexible options, I don't mind if you want to add a NewBotWithPassword that does what you need.

Hopefully you also don't need proxy and it doesn't collide with it?

[StalkR]

Re: multi network, I guess it's not a problem, you could run separate bot instances b.Run() in one goroutine, then a second one in another, etc. But if you plug the same plugin to multiple bot instances, it needs to handle working with multiple bots... not a given but doable.

[quite]

In this case, and following the previous idea for proxy, I'd have to rename the function that actually does the work to NewBothWithProxyPassword (and adjust for that) and introduce the NewBotWithPassword fronting that. Easy to do, but already beginning to look ugly :)

It would be neat to implement "functional options", like Dave Cheney does in https://github.com/pkg/term But I think that would require moving all of the code that actually connects from NewBot() to Run(). One could then do something like:

b := bot.NewBotOptions(host, bot.Nick("foo"), bot.Proxy("bar"), bot.Password("gazonk"), …)
b.Run()

And the currently helpers NewBot() and NewBotWithProxy() could be kept of course, if rewritten to use NewBotOptions() internally.

[quite]

Re:multi network, I haven't thought that far yet, just began furnishing for running multiple bots :) The core of my plugin, doListen() takes functions that say or quit. The setup of the core goroutine, Listen(), only uses a bot instance in-scope to prepare those functions. I think it might be workable, but we'll see.

[quite]

I tried implementing functional options in a branch. Including changes to the exampleBot.

It's quite neat. I decided that host and nick are the only options that are always required, but that could definitely be discussed.

Of ourse, it would be neat if we only had NewBot() that takes options, but yeah, the API... I also saw #12 which would actually break the API anyway. And would as well have consequences for how to actully design the functional options API...

[quite]

I already found out that I need to tweak the SSLConfig: tls.Config(hostPort[0]) (to set InsecureSkipVerify for a case of self-signed certificate). That'd be another option-func, maybe SSLInsecure()...

Currently, I was thinking that the option-funcs does not need to return errors, but perhaps some would at some point. In that case, the new bot function that takes options (currently NewBotOptions()) would need to return error, an API change. So we need to consider that as well :)

[StalkR]

Cool, thanks for looking into this - send me the PR when ready :)

Yes https://commandcenter.blogspot.com/2014/01/self-referential-functions-and-design.html or functional options is what I had in mind as well, but never got around to do it.

I was thinking that the option-funcs does not need to return errors, but perhaps some would at some point. In that case, the new bot function that takes options (currently NewBotOptions()) would need to return error, an API change. So we need to consider that as well :)

I'm not sure we'll need options returning errors, but yes changing NewBotOptions to return any initialization error sounds good. And we can't defer them to Run later on, as it does not return errors because it runs as a loop catching errors and retrying.

I also saw #12 which would actually break the API anyway. And would as well have consequences for how to actully design the functional options API...

Right, I forgot about that one. It came up so a client could connect to server A/B and if A is down it continues to B (instead of relying entirely on having a DNS with round-robin on A/B). It could be implemented by having an option for bot.Host() and if provided multiple times, becomes the list?

I already found out that I need to tweak the SSLConfig: tls.Config(hostPort[0]) (to set InsecureSkipVerify for a case of self-signed certificate). That'd be another option-func, maybe SSLInsecure()...

I'd rather not provide this option, there is no need to do things insecurely, even if a server uses self-signed certificates. I added the -cacerts options exactly for this use case https://github.com/StalkR/goircbot/blob/master/lib/tls/tls.go#L14 to be used like this:

cat /etc/ssl/certs/ca-certificates.crt cert-of-irc-server.pem > certs.pem
go build bot.go
./bot -cacerts certs.pem

[quite]

Yes https://commandcenter.blogspot.com/2014/01/self-referential-functions-and-design.html or functional options is what I had in mind as well, but never got around to do it.

Is there any point of going all the way and making options self-referential like Pike does?

I'm not sure we'll need options returning errors, but yes changing NewBotOptions to return any initialization error sounds good. And we can't defer them to Run later on, as it does not return errors because it runs as a loop catching errors and retrying.

Right. I'll let NewBotOptions return also an error. If we'll need options returning errors, we could actually add that later without breaking the API, since the function is taking an arbitary number of instances of something that we control. Just changing the options ...func(*BotImpl) part of the signature, right. I guess this would apply to the above, self-ref, as well then.

Right, I forgot about that one. It came up so a client could connect to server A/B and if A is down it continues to B (instead of relying entirely on having a DNS with round-robin on A/B). It could be implemented by having an option for bot.Host() and if provided multiple times, becomes the list?

Got it. And yes, Host() could work, but I'll leave that for now. But, we need to settle on the non-func-options of NewBotOptions(). I thought they could be only host string, nick string, so complete signature:

func NewBotOptions(host string, nick string, options ...func(*BotImpl)) (Bot, error)

What do you think? These two are always required, so somehow they should perhaps be non-functional? But If we add a Host() func, then it'd feel slightly skewed having to first provide the non-optional host string... hm.

I'd rather not provide this option, there is no need to do things insecurely, even if a server uses self-signed certificates. I added the -cacerts options exactly for this use case https://github.com/StalkR/goircbot/blob/master/lib/tls/tls.go#L14 to be used like this:

You're right. I never got it to work properly in this manner, but I know it really should work. I need to try it out again.

[StalkR]

Is there any point of going all the way and making options self-referential like Pike does?

I'm more familiar with the self-referential model and using it, also it seems better to have an option type to group them together. It's used in various projects, e.g. https://godoc.org/google.golang.org/grpc#DialOption

we need to settle on the non-func-options of NewBotOptions()

Right, and there's precedent for non-option required parameters (https://godoc.org/google.golang.org/grpc#Dial has target string) so we could go that way...

These two are always required, so somehow they should perhaps be non-functional?

The other options (ident, real name, SSL or not, version, etc.) are required ones for which we decided a default, so why making nick special, we could also decide a default for the nick (bot) and make it an option. It's probably not what people want but it works. Only host is really required, because making it default irc.example.com would not work.

But If we add a Host() func, then it'd feel slightly skewed having to first provide the non-optional host string... hm.

Agreed...

So how about we make everything is an option, and the constructor can bail out early if some required set of conditions aren't met: no Host option, conflicting options, etc.

Bonus of making host an option, we can choose a signature allowing 1 or more hosts: Host(host string, other ...string) -- works with Host("irc.example.com") as well as Host("irc.example.com", "irc2.example.com") and more, which I find nicer than taking a slice if you only have one host: Host([]string{"irc.example.com"}).

Added bonus is order of options does not matter, people can organize them however they want, and it's explicit what they are by the option call, unlike function parameters which aren't named.

[quite]

Right. So grpc lets the option functions return a struct which implements an option interface. But it is not really self-referential -- it does not return a new option which can be used to set the option to its previous value, which is what Mr Pike on the other hand eventually accomplishes. But the latter would feel a bit over the top for our usecase here.

[quite]

Other than that, I'm good with it's-all-options. Returning error if no host set, goircbot as default nick.(ident and realname are taken from the nick if empty -- e.g. freenode doesn't admin registration if realname is empty!). Yes, host can then be extended with other ...string without breaking API.

unlike function parameters which aren't named.

Ah hm, what I meant with functional parameters all along, and what I implemented, are indeed named function inside the lib... There was never any case of taking use-constructed functions for setting stuff. If I understood correctly how you might have misunderstood me :) Like:

func SSL(ssl bool) func(*BotImpl) {
       return func(b *BotImpl) { b.config.SSL = ssl }

[StalkR]

Re: self-referential so option can be set to previous value, ok agreed we don't need that. Functional is enough here.

Re: all options, cool let's do it!

ident and realname are taken from the nick if empty -- e.g. freenode doesn't admin registration if realname is empty

Ha freenode! All set to nick is a reasonable default. We could set ident to $USER like other irc client software do but I never liked that it leaked my username if I wasn't careful, so I prefer all from nick.

Ah hm, what I meant with functional parameters all along, and what I implemented, are indeed named function inside the lib... There was never any case of taking use-constructed functions for setting stuff. If I understood correctly how you might have misunderstood me :)

No I understood you :) I just had a realization regarding parameters naming and order:

See func New(nick, ident string) because parameters aren't named when they are called, you don't see what is what just by looking at the call: New("john", "smith") is john the nick or the ident? need to look at the API. Now compare to Python New(nick="john", ident="smith"), it's explicit and could also be with different argument order New(ident="smith", nick="john"). Some C/C++ styles do with New(/*nick=*/"john", /*ident=*/"smith"), could work in Go actually but ugh. So I like it that with options it's not only explicit but argument order also doesn't matter: New(Nick("john"), Ident("smith")) or New(Ident("smith"), Nick("john")).

[quite]

Great. What I have since last hacking (but not pushed yet) is what we now have agreed upon :) But I'll need to polish it slightly, and consider your review comments.

Yeah, that's my take too on the pro's of functional options. Explicit and order-insensitive api, much like Python keyword args.

[quite]

Concerning #12, I think that the current option signature Host(host string) is fine. It can be extended with ...other string. Alternatively rewritten so a second Host("example:123") option extends the previous. Either way, no breaking API change :)

[StalkR]

Agreed!

[StalkR]

Fixed by PR #21 - thanks!

[StalkR]

Turns out moving the conn initialization outside of New and inside of Run (via setup) is breaking plugins who expected to have a valid conn after New.

https://github.com/StalkR/goircbot/blob/master/plugins/invite/handlers.go#L11 conn is nil so Conn() returns nil and boom

This is how I tested:

go run $GOPATH/src/github.com/StalkR/goircbot/examplebot.go -host=irc.stalkr.net -channels=#dev -nick=godev

[StalkR]

Fixed in #22