[Code-review] review of Daf's misc gadget branch
Guillaume Desmottes
guillaume.desmottes at collabora.co.uk
Thu Jun 26 04:16:47 EDT 2008
Review of
https://dev.laptop.org/git?p=users/daf/gadget;a=shortlog;h=misc
> diff --git a/gadget.tac b/gadget.tac
> index 353c3aa..ce33782 100644
> --- a/gadget.tac
> +++ b/gadget.tac
> @@ -7,7 +7,23 @@ from gadget.config import read_config
> from gadget.model import GadgetModel
> from gadget.roster import GadgetRoster
>
> -config = read_config('gadget.config')
> +def read_config_():
IMHO it's confusing to use the same method name with just a '_' as
suffix (I didn't notice it when I first read the code). Would be better
to use a different name. read_configuration() maybe?
Except that, patch looks good.
> + config = None
> +
> + for path in ('gadget.config', '/etc/gadget/gadget.config'):
> + try:
> + config = read_config(path)
> + except IOError:
> + continue
> + else:
> + break
> +
> + if config is None:
> + raise RuntimeError("failed to find configuration file")
> +
> + return config
> +
> +config = read_config_()
> domain = config['domain']
> password = config['password']
> strport = 'tcp:%s:%d' % (config['server_host'],
config['server_port'])
> diff --git a/gadget/component.py b/gadget/component.py
> index f082f3b..b64318e 100644
> --- a/gadget/component.py
> +++ b/gadget/component.py
> @@ -587,7 +587,7 @@ class GadgetService(component.Service):
>
> elif type == 'probe':
> log.msg('%s is online' % jid)
> - self.model.buddy_add(jid, {})
> + self.model.create_buddy(jid, {})
>
> self.send_presence(to, from_)
>
> @@ -681,7 +681,7 @@ class GadgetService(component.Service):
> if self.model.has_buddy(jid):
> self.model.buddy_update(jid, props)
> else:
> - self.model.buddy_add(jid, props)
> + self.model.create_buddy(jid, props)
>
> # FIXME: send notification messages to interested buddies
>
> diff --git a/gadget/model.py b/gadget/model.py
> index 5b23a2a..226778e 100644
> --- a/gadget/model.py
> +++ b/gadget/model.py
> @@ -72,7 +72,7 @@ class GadgetModel(object):
> view.results.append(activity)
> view.handler.activity_found(activity)
>
> - def activity_add(self, id, room, properties):
> + def create_activity(self, id, room, properties):
> activity = Activity(id, room, properties)
> self.add_activity(activity)
> return activity
> @@ -205,9 +205,12 @@ class GadgetModel(object):
> view.results = results
> return view
>
> - def buddy_add(self, jid, properties):
> + def add_buddy(self, buddy):
> + self.buddies[buddy.jid] = buddy
> +
> + def create_buddy(self, jid, properties):
> buddy = Buddy(jid, properties)
> - self.buddies[jid] = buddy
> + self.add_buddy(buddy)
> return buddy
>
> def buddy_update(self, jid, properties):
> diff --git a/gadget/test_activity_query.py
b/gadget/test_activity_query.py
> index 5a8e36f..5d0b0cf 100644
> --- a/gadget/test_activity_query.py
> +++ b/gadget/test_activity_query.py
> @@ -56,8 +56,8 @@ class ActivityIqTest2(TestCase):
> self.server.addOnetimeObserver('//iq', self.iq)
>
> model = self.service.model
> - model.activity_add(1, 'foo', {})
> - model.activity_add(2, 'bar', {})
> + model.create_activity(1, 'foo', {})
> + model.create_activity(2, 'bar', {})
>
> iq = IQ(self.server, "get")
> iq['from'] = 'bob at foo.org'
> @@ -80,7 +80,7 @@ class ActivityIqTest3(TestCase):
> self.server.addOnetimeObserver('//iq', self.iq)
>
> model = self.service.model
> - activity = model.activity_add(1, 'foo', {})
> + activity = model.create_activity(1, 'foo', {})
> activity.members.add('bob at foo.org')
> activity.members.add('tom at foo.org')
>
> @@ -107,9 +107,9 @@ class ActivityIqTest4(TestCase):
> self.server.addOnetimeObserver('//iq', self.iq)
>
> model = self.service.model
> - model.activity_add(1, 'foo', {'type': ('str', 'Paint')})
> - model.activity_add(2, 'bar', {'type': ('str', 'Connect')})
> - model.activity_add(3, 'test', {'type': ('str', 'Paint')})
> + model.create_activity(1, 'foo', {'type': ('str', 'Paint')})
> + model.create_activity(2, 'bar', {'type': ('str', 'Connect')})
> + model.create_activity(3, 'test', {'type': ('str', 'Paint')})
>
> iq = IQ(self.server, "get")
> iq['from'] = 'bob at foo.org'
> @@ -138,9 +138,9 @@ class ActivityIqTest5(TestCase):
> self.server.addOnetimeObserver('//iq', self.iq)
>
> model = self.service.model
> - model.activity_add(1, 'foo', {'type': ('str', 'Paint')})
> - model.activity_add(2, 'bar', {'type': ('str', 'Connect')})
> - model.activity_add(3, 'test', {'type': ('str', 'Paint')})
> + model.create_activity(1, 'foo', {'type': ('str', 'Paint')})
> + model.create_activity(2, 'bar', {'type': ('str', 'Connect')})
> + model.create_activity(3, 'test', {'type': ('str', 'Paint')})
>
> iq = IQ(self.server, "get")
> iq['from'] = 'bob at foo.org'
> @@ -165,12 +165,12 @@ class ActivityIqTest6(TestCase):
> self.server.addOnetimeObserver('//iq', self.iq)
>
> model = self.service.model
> - activity = model.activity_add(1, 'foo', {'type': ('str',
'Paint')})
> + activity = model.create_activity(1, 'foo', {'type': ('str',
'Paint')})
> activity.members.add('alice at foo.org')
> activity.members.add('bob at foo.org')
> - activity = model.activity_add(2, 'bar', {'type': ('str',
'Connect')})
> + activity = model.create_activity(2, 'bar', {'type': ('str',
'Connect')})
> activity.members.add('alice at foo.org')
> - activity = model.activity_add(3, 'test', {'type': ('str',
'Paint')})
> + activity = model.create_activity(3, 'test', {'type': ('str',
'Paint')})
> activity.members.add('bob at foo.org')
>
> iq = IQ(self.server, "get")
> @@ -199,12 +199,12 @@ class ActivityIqTest7(TestCase):
> self.server.addOnetimeObserver('//iq', self.iq)
>
> model = self.service.model
> - activity = model.activity_add(1, 'foo', {'type': ('str',
'Paint')})
> + activity = model.create_activity(1, 'foo', {'type': ('str',
'Paint')})
> activity.members.add('alice at foo.org')
> activity.members.add('bob at foo.org')
> - activity = model.activity_add(2, 'bar', {'type': ('str',
'Connect')})
> + activity = model.create_activity(2, 'bar', {'type': ('str',
'Connect')})
> activity.members.add('alice at foo.org')
> - activity = model.activity_add(3, 'test', {'type': ('str',
'Paint')})
> + activity = model.create_activity(3, 'test', {'type': ('str',
'Paint')})
> activity.members.add('bob at foo.org')
>
> iq = IQ(self.server, "get")
> @@ -237,12 +237,12 @@ class ActivityIqTest8(TestCase):
> self.server.addOnetimeObserver('//iq', self.iq)
>
> model = self.service.model
> - activity = model.activity_add(1, 'foo', {'type': ('str',
'Paint')})
> + activity = model.create_activity(1, 'foo', {'type': ('str',
'Paint')})
> activity.members.add('alice at foo.org')
> activity.members.add('bob at foo.org')
> - activity = model.activity_add(2, 'bar', {'type': ('str',
'Connect')})
> + activity = model.create_activity(2, 'bar', {'type': ('str',
'Connect')})
> activity.members.add('alice at foo.org')
> - activity = model.activity_add(3, 'test', {'type': ('str',
'Paint')})
> + activity = model.create_activity(3, 'test', {'type': ('str',
'Paint')})
> activity.members.add('bob at foo.org')
>
> iq = IQ(self.server, "get")
> @@ -306,8 +306,8 @@ class ActivityViewByProperties(TestCase):
> self.server.addOnetimeObserver('//iq', self.iq)
>
> model = self.service.model
> - model.activity_add(1, 'foo', {})
> - model.activity_add(2, 'bar', {'color': ('str', 'blue')})
> + model.create_activity(1, 'foo', {})
> + model.create_activity(2, 'bar', {'color': ('str', 'blue')})
>
> iq = IQ(self.server, "get")
> iq['from'] = 'bob at foo.org'
> @@ -352,9 +352,9 @@ class ActivityIqRandomQuery(TestCase):
> self.server.addOnetimeObserver('//iq', self.iq)
>
> model = self.service.model
> - model.activity_add(1, 'foo', {})
> - model.activity_add(2, 'bar', {})
> - model.activity_add(3, 'barbar', {})
> + model.create_activity(1, 'foo', {})
> + model.create_activity(2, 'bar', {})
> + model.create_activity(3, 'barbar', {})
>
> iq = IQ(self.server, "get")
> iq['from'] = 'bob at foo.org'
> diff --git a/gadget/test_buddy_query.py b/gadget/test_buddy_query.py
> index 9973789..90bd29f 100644
> --- a/gadget/test_buddy_query.py
> +++ b/gadget/test_buddy_query.py
> @@ -59,8 +59,8 @@ class BuddyIqTest2(TestCase):
> self.server.addOnetimeObserver('//iq', self.iq)
>
> model = self.service.model
> - model.buddy_add('foo', {})
> - model.buddy_add('bar', {})
> + model.create_buddy('foo', {})
> + model.create_buddy('bar', {})
>
> iq = IQ(self.server, "get")
> iq['from'] = 'bob at foo.org'
> @@ -83,8 +83,8 @@ class BuddyIqTest3(TestCase):
> self.server.addOnetimeObserver('//iq', self.iq)
>
> model = self.service.model
> - model.buddy_add('alice at foo.org', {'color': ('str',
'#005FE4,#00A0FF')})
> - model.buddy_add('marc at foo.org', {})
> + model.create_buddy('alice at foo.org', {'color': ('str',
'#005FE4,#00A0FF')})
> + model.create_buddy('marc at foo.org', {})
>
> iq = IQ(self.server, "get")
> iq['from'] = 'bob at foo.org'
> @@ -110,8 +110,8 @@ class BuddyIqTest4(TestCase):
> self.server.addOnetimeObserver('//iq', self.iq)
>
> model = self.service.model
> - model.buddy_add('alice at foo.org', {'color': ('str',
'#005FE4,#00A0FF')})
> - model.buddy_add('marc at foo.org', {})
> + model.create_buddy('alice at foo.org', {'color': ('str',
'#005FE4,#00A0FF')})
> + model.create_buddy('marc at foo.org', {})
>
> iq = IQ(self.server, "get")
> iq['from'] = 'bob at foo.org'
> @@ -141,9 +141,9 @@ class BuddyIqTest5(TestCase):
> self.server.addOnetimeObserver('//iq', self.iq)
>
> model = self.service.model
> - model.buddy_add('alice at foo.org', {'color': ('str',
'#005FE4,#00A0FF')})
> - model.buddy_add('marc at foo.org', {})
> - model.buddy_add('jean at foo.org', {})
> + model.create_buddy('alice at foo.org', {'color': ('str',
'#005FE4,#00A0FF')})
> + model.create_buddy('marc at foo.org', {})
> + model.create_buddy('jean at foo.org', {})
>
> iq = IQ(self.server, "get")
> iq['from'] = 'bob at foo.org'
> @@ -207,10 +207,10 @@ class BuddyIqRandomQuery(TestCase):
> self.server.addOnetimeObserver('//iq', self.iq)
>
> model = self.service.model
> - model.buddy_add('alice at foo.org', {})
> - model.buddy_add('bob at foo.org', {})
> - model.buddy_add('charles at foo.org', {})
> - model.buddy_add('max at foo.org', {})
> + model.create_buddy('alice at foo.org', {})
> + model.create_buddy('bob at foo.org', {})
> + model.create_buddy('charles at foo.org', {})
> + model.create_buddy('max at foo.org', {})
>
> iq = IQ(self.server, "get")
> iq['from'] = 'bob at foo.org'
> diff --git a/gadget/test_component.py b/gadget/test_component.py
> index 5890486..2d8089c 100644
> --- a/gadget/test_component.py
> +++ b/gadget/test_component.py
> @@ -7,8 +7,8 @@ from twisted.words.protocols.jabber import component,
xmlstream
> from twisted.words.xish import domish
> from twisted.words.protocols.jabber.client import IQ
>
> -from component import GadgetService, parse_properties,
properties_to_xml,\
> - PropertyTypeError
> +from component import (
> + GadgetService, PropertyTypeError, parse_properties,
properties_to_xml)
> from model import GadgetModel, Activity
>
> import ns
> @@ -172,7 +172,7 @@ class UnavailablePresenceTest(TestCase):
> unavailable presence"""
> def runTest(self):
> self.done = defer.Deferred()
> - self.service.model.buddy_add('bob at foo.org', {})
> + self.service.model.create_buddy('bob at foo.org', {})
>
> presence = domish.Element((ns.CLIENT, 'presence'))
> presence['from'] = 'bob at foo.org'
> @@ -482,7 +482,7 @@ class CapsTest(TestCase):
> class BuddyPropertiesChangeTest(TestCase):
> def runTest(self):
> self.done = defer.Deferred()
> - self.service.model.buddy_add('bob at foo.org', {})
> + self.service.model.create_buddy('bob at foo.org', {})
>
> message = domish.Element((None, 'message'))
> message['from'] = 'bob at foo.org'
> diff --git a/gadget/test_model.py b/gadget/test_model.py
> index 0ce5364..51d852a 100644
> --- a/gadget/test_model.py
> +++ b/gadget/test_model.py
> @@ -7,7 +7,7 @@ class ModelTest(unittest.TestCase):
> def test_activity_search(self):
> model = GadgetModel()
> assert not model.has_activity('123')
> - activity = model.activity_add('123', 'room at conf.foo.org',
> + activity = model.create_activity('123', 'room at conf.foo.org',
> {'type': ('str', 'Paint')})
> assert model.has_activity('123')
>
> @@ -32,7 +32,7 @@ class ModelTest(unittest.TestCase):
> results = model.activity_by_participants(['boo at foo.org'])
> self.assertEquals([], results)
>
> - activity2 = model.activity_add('456', 'room2 at conf.foo.org',
{})
> + activity2 = model.create_activity('456',
'room2 at conf.foo.org', {})
> activity2.members.add('alice at foo.org')
> activity2.members.add('bob at foo.org')
> results = model.activity_by_participants(['alice at foo.org'])
> @@ -55,7 +55,7 @@ class ModelTest(unittest.TestCase):
>
> assert not model.has_buddy('alice at foo.org')
> self.assertRaises(KeyError, model.buddy_by_id,
'alice at foo.org')
> - alice = model.buddy_add('alice at foo.org', {})
> + alice = model.create_buddy('alice at foo.org', {})
> assert model.has_buddy('alice at foo.org')
>
> result = model.buddy_by_id('alice at foo.org')
> @@ -76,8 +76,8 @@ class ModelTest(unittest.TestCase):
> results = model.buddy_search('bob at foo.org', None)
> self.assertEquals([], list(results))
>
> - bob = model.buddy_add('bob at foo.org', {'color': ('str',
'red')})
> - charles = model.buddy_add('charles at foo.org', {'color':
('str', 'red')})
> + bob = model.create_buddy('bob at foo.org', {'color': ('str',
'red')})
> + charles = model.create_buddy('charles at foo.org', {'color':
('str', 'red')})
> results = model.buddy_search(None, {'color': ('str', 'red')})
> self.assertEquals([bob, charles], sorted(results))
>
> @@ -86,8 +86,8 @@ class ModelTest(unittest.TestCase):
>
> def test_buddy_random(self):
> model = GadgetModel()
> - model.buddy_add('alice at foo.org', {})
> - model.buddy_add('bob at foo.org', {})
> + model.create_buddy('alice at foo.org', {})
> + model.create_buddy('bob at foo.org', {})
>
> result = model.buddy_random(1)
> assert len(result) == 1
> @@ -100,9 +100,9 @@ class ModelTest(unittest.TestCase):
>
> def test_activity_random(self):
> model = GadgetModel()
> - model.activity_add('123', 'room1 at conf.foo.org', {})
> - model.activity_add('456', 'room2 at conf.foo.org', {})
> - model.activity_add('789', 'room3 at conf.foo.org', {})
> + model.create_activity('123', 'room1 at conf.foo.org', {})
> + model.create_activity('456', 'room2 at conf.foo.org', {})
> + model.create_activity('789', 'room3 at conf.foo.org', {})
>
> result = model.activity_random(2)
> assert len(result) == 2
> @@ -130,7 +130,7 @@ class ModelTest(unittest.TestCase):
> removed_log.append((activity, buddy))
>
> model = GadgetModel()
> - activity1 = model.activity_add(123, 'room1 at conf.foo.org',
> + activity1 = model.create_activity(123, 'room1 at conf.foo.org',
> {'color': ('str', 'red')})
> view = model.activity_view(3, {'color': ('str', 'red')},
ViewHandler())
>
> @@ -139,16 +139,16 @@ class ModelTest(unittest.TestCase):
> self.assertEquals([], gone_log)
>
> # Non-matching activity added to model.
> - activity2 = model.activity_add(456, 'room2 at conf.foo.org', {})
> + activity2 = model.create_activity(456, 'room2 at conf.foo.org',
{})
>
> self.assertEquals([activity1], view.results)
> self.assertEquals([], found_log)
> self.assertEquals([], gone_log)
>
> # Matching activities added to model.
> - activity3 = model.activity_add(768, 'room3 at conf.foo.org',
> + activity3 = model.create_activity(768, 'room3 at conf.foo.org',
> {'color': ('str', 'red')})
> - activity4 = model.activity_add(901, 'room4 at conf.foo.org',
> + activity4 = model.create_activity(901, 'room4 at conf.foo.org',
> {'color': ('str', 'red')})
>
> self.assertEquals([activity1, activity3, activity4],
view.results)
More information about the Code-review
mailing list