Code review comment for lp:~cjcurran/friends/another-test-fix-contacts-fb-import

Revision history for this message
Robert Bruce Park (robru) wrote :

Ok Conor, I've landed your branch, it is looking a lot better and the tests are now passing. Please delete that old branch that has the stale mp still outstanding, it looks bad just sitting there with a 'needs fixing' review seeing as it's now fixed and landed ;-)

One thing I just want to mention though, the Facebook._create_contact method is quite long, and only about half of it seems to be specific to Facebook. Ideally, what I would like to see is that you implement a method called Base._create_contact that handles all of the EBook logic, and then you'd have a very tiny wrapper around that from the Facebook plugin that just inserts the correct data into it. I'd like to see facebook.py not even import EBook at all (notice how it doesn't need to import any UOA modules despite implementing a ton of UOA logic).

Doing that will actually make it a lot easier to move forward with Twitter contacts, because you'll have more code re-use in the Base class and less code duplicated between protocols.

So when you start on the Twitter contacts, please don't even start importing EBook from twitter.py, first start working by dropping the EBook import from facebook.py and then work around that by wrapping it from the Base class.

Thanks!

review: Approve

« Back to merge proposal