A few comments:

On 29/07/2013 23:27, Martin Morrison wrote:
> Fair enough - the first 5 lines was what I looked at. ;-) You have also, of course, lost the awesome HHGG quote that gets logged when initialisation is complete...
The HHGG quote was nice but when room configuration moved to a plugin it 
tended to fire before everything was configured so wasn't really 
accurate any more.
> Further comments:
>
> - Might be worth censoring the manhole dict in practice. Pick the elements that are most useful, given your experience. Might also be worth having a whole separate module with some "helper functions" that get put into the manhole dict, to make it even more useful (at this stage, no need for the extra helpers, but deferring to a module that could set them up would be useful).
I've censored the manhole dict a bit and moved some more stuff into 
manhole.py - not sure if this is what you meant but there should be room 
to add helper functions in manhole.py now
> - Related: instead of listening for the manhole in this module, defer entirely to the manhole module (something like "start_manhole(args, droid)").
>
> - __main__.py made executable - also not necessary.
>
> configparser.py
>
> - There are some "@@@" comments in the configparser. These should be converted into something more appropriate (like actual docstrings, or potentially bug reports/feature requests in launchpad).
>
> - Line 421: typo (Pasrer).
>
> - [Minor: method names should be underscore_separated and not camelCase (I know existing code is wrong: new code should follow the coding standards though)]
>
> - This module (configparser) could do with some (a lot ;-)) of docstrings, in particular explaining the expected/supported syntax for section names and aliases.
>
> - The get method can be made more efficient:
>
>   . Don't create lots of intermediate lists. Use generator expressions instead, and add the if to the same generator expression. (you'll need to make it into a list at the end)
I tried to make the list comprehension into a generator expression in 
the .get method but I just got empy lists back - not entirely sure why 
at the moment, will have more of a look this afternoon.
>   . Instead of the nested try statements, use simpler if statements. It makes the code a bit clearer.
>   . A few newlines wouldn't go amiss. Some use of kwargs.pop(key, None) would also save the trys.
>   . Why are the copies shallow? What if the value contains a list or dict?
>
> - Parser.splitter should probably be Parser.SPLITTER (effectively a constant)
>
> cron.py
>
> - Lots of whitespace changes, without any real content. A good opportunity to improve more docstrings. ;-)
>
> manhole.py
>
> - def manhole_factory. Or, better, change to start_manhole as mentioned above.
>
> - Comment about changing perhaps should be removed? For now, I think this is fine.
>
> messagehandler.py
>
> - Also executable (I'll stop adding this comment. Please fix all files)
>
> - Use of im_self in the Handler class is dangerous! Assumes that plugins will only ever pass *methods* of *themselves* - both of which can quite easily not be true. This needs removing.
>
> - The way handlers are now stored, they don't sort by priority! This is bad. Needs reverting (either by pulling priority out of the Handler object, or by doing extra work in that class to make it sort correctly).
>
> - You've reordered the dicts in the handlers storage - why? Just means all the functions have to change.
I had hoped that this would make plugin reloading more possible but as 
that never happened it hasn't really been very useful. Might be handy in 
future for deregistering plugins in future.
>
> - Apart from a rename of 'mtype' to 'place', send() and reply() are identical. Why make them look so changed? :-)
>
> - You've altered the constructor of Message - why? Just means more changes everywhere that we create them.
>
> - Inside the Message class, you've made __handlers not private anymore - why is that? Probably wants to be private again. Also this forms part of the external API, and is used by Command and PatternMatch.
>
> In general, feels like this module has changed a lot with almost no change in functionality. I think it would be clearer to revert most of it to minimise the diff.
>
> pluginmanager.py
>
> - You've changed the docstring for PluginMeta - but not changed the content! Why? Revert.
>
> - Similarly for the implementation - no visible changes, just variable name changes. Revert them.
I have left the change of parameter name from dict to dictionary
> - Register functions again seem largely unchanged in behaviour. Any minor improvements to the docstrings can be made in a separate commit where it's possible to distinguish them from behavioural changes.
The only thing that has changed is that the call goes to self._register 
(a wrapper around self.messaghandler.register_callback) rather than to 
self._pm.register_*_callback (things go straight to messagehandler 
rather than via pluginmanager)
The doc changes did little except cut out about 80 lines or so of 
duplicated docstrings.
>
> - The additional dependency tracking in the get function - what is it for? By this point, it is too late (as the plugins have already been loaded). Further, the PluginProxy is meant to allow plugins to operate even if their dependencies are not loaded. So this just seems unnecessary?
>
> - Why have you removed the default endroid_init() method? Now all plugins are forced to override it.
>
> - The extra check for modname in sys.modules is unnecessary. The __import__ function does this for you. Revert.
>
> - You've changed the behaviour of the PluginProxy so that fetching non-existent attributes will return an exception. This goes against what the Proxy is trying to do. Probably want to revert that.
>
> [Side note: the original intention wasn't to wrap all plugins in a PluginProxy, but only plugins that don't exist. This might be a nice separate change to make, but not as part of this wider change]
>
> blacklist.py
>
> - The changes turn it into a global blacklist. It should be possible to do that without having to hit the whole database on every check. Make _blacklist a class variable.
>
> chuck.py
>
> - You seem to have changed it so that the docstring is the helpstring. This probably isn't quite right, as the former is for any developers changing the code, while the latter is for folks using the plugin via XMPP.
>
> - Random commented out line added.
>
> command.py
>
> - Only whitespace (and perms) changes: revert!
>
> coolit.py
>
> - No functional change: revert!
>
> correct.py
>
> - Note that the API change makes it much harder to use. Why not provide something more like the original API as well?
I could add a sendernick attribute to the message class though that 
would increase the reliance of messagehandler on usermanagement. The new 
method is more verbose but will get up-to-date nicknames for users in 
rooms too (ie allowing for nickname changes)
>
> echobot.py
>
> - You've reverted the change to the new method names.
>
> help.py
>
> - Whitespace change. Revert.
>
> httpinterface.py
>
> - Whitespace changes only: revert!
>
> invite.py
>
> - Alignment in this file is all wacky. You seem to have used tabs. Please ensure there are no tabs in any files! Also missing newline at end of this file.
>
> remote.py
>
> - In the send_chat() API, you seem to have added an extra arg (that is None). What is this? Why not make it optional?
>
> roomowner.py
>
> - Tabs again!
>
> - I think this plugin needs to do some sanitisation of its configuration before blindly passing it in to XMPP.
This is done lower down in the call to usermanagement.configure
> spell.py
>
> - Seems Google spell may be broken. How sad. Think we can ignore this for now (remove the comment from this diff though)
>
> theyfightcrime.py
>
> - revert
>
>
> [More review comments tomorrow. As for what do you: you can push a new change anytime you want, and update/resubmit the review when you're ready]