Code review comment for lp:~rockstar/entertainer/entertainer-server

Revision history for this message
Matt Layman (mblayman) wrote :

I know you said you'll be merging this into some other branch, so I'm approving on those grounds (because there looks to be some things that you just wouldn't merge onto a production trunk), but here are my comments.

1) Port number stuff is already in the Configuration class. That could be used, but in the long run I don't know how we could "announce" that a server is available to connect to (is that something that would happen if we eventually add uPnP support?).

2) I'm assuming that pylint W0223 will be removed as soon as LocalStorage actually implements the abstract Storage methods. I think that could benefit from a XXX comment to help remember.

3) pylint R0922 could also benefit from a XXX comment in the pylintrc file. I don't think that the intention would be to remove it permanently as it seems to add value (especially if we start using more abstract classes).

review: Approve

« Back to merge proposal