widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Nice. That is a really good idea and I think the implementation looks reasonable too. It is a bit drafty still, though (see comments below). Was this the first time you did something with go? How did you like it?

Some comments:

- func NewIRCBridge() IRCBridge should read its configuration from the same json file where we read the data from right now too (see main.go).
- I think the design is a little fragile - if something goes wrong with the IRC connection the whole metaserver comes down. How about running the IRC bridge in a go routine that gets a (buffered) channel of stuff to send and also sends stuff into a channel that the metaserver then displays? That way if the bridge is disconnected it can just discard the messages and the whole design is a bit decoupled. Also you can fake some IRC messages in a test and test the whole thing.
- +func (s *Server) Motd() string - I do not agree with this change. When a method is not changing a struct, it should not be a pointer receiver. There are only a few exceptions to this rule as far as I understood go so far (not an expert by any means). 
- Please add some tests. Feel free to add end to end tests, there are already a few in the metaserver. 
-- 
https://code.launchpad.net/~widelands-dev/widelands-metaserver/ircbridge/+merge/203277
Your team Widelands Developers is subscribed to branch lp:widelands-metaserver.