Code review comment for lp:~ben-hutchings/endroid/roomowner

Revision history for this message
Martin Morrison (isoschiz) wrote :

Looking pretty good now! A few comments below, but nothing major.

bin/endroid:

- Given we are frigging with the PYTHONPATH anyway, we should add "~/.endroid/plugins/" to it, *before* the /usr/lib/ entry.

- This can also change to python -m endroid now that we have __main__.py

endroid.conf

- The plugins were/are listed alphabetically. Move roomowner to the right place.

- There's a missing newline at the end of the file

- It's odd to have example roomowner plugin config in here, but no other plugins. Probably move this to the roomowner docs instead.

setup.py

- This file wants to keep its +x permissions ;-)

endroid/__init__.py

- Line 2026 can go (extra LOGGING_FORMAT)

- Logging for groups and rooms is noisy (many lines). Simple change to ", ".join(rooms) will improve that.

endroid/confparser.py

- The literal_eval thing is cool. Except when it comes to bools. This should be made clear in the docstring.

- get() doesn't need to take arbitrary kwargs. If it does though, it should verify that it doesn't get any unexpected ones (swallowing get("foo", defult=True) silently is Evil)

cron.py

- The description of cron in the docstring is a bit misleading. Need to be explicit that .register() returns a Task object on which the later funcs can be called.

- There's no real need to change the table field name from fun_name to reg_name (even if it is more accurate). It just makes it not backwards compatible. Revert this.

- Docstring on register is not formatted correctly. It also doesn't really explain that it returns a Task object that can be used to get the callback called in the future.

- In removeTask instead of pop, use del.

database.py

- Database docstring not formatted correctly.

manhole.py

- start_manhole docstring not formatted correctly.

blacklist.py

- The get_blacklist() method is superfluous. Can get rid of it and access _blacklist directly

- Use discard instead of remove (list 3839)

broadcast.py

- Think this should be added separately in a different branch.

invite.py

- Plugin should be named Invite to be consistent!

- Helphints need the command keywords removing (they are already added by Command)

rosterhandler.py

- in purge_roster (in connectionInitialized) no need to pass self through - it's a closure with access to that anyway.

- You've added a set_available() method that does nothing. Remove it again.

usermanagement.py

- The default roomname shouldn't be Ensoft. EnDroid would do.

« Back to merge proposal