[Sugar-devel] [PATCH sugar-toolkit] presence: use RoomConfig1 to configure channel properties (#3629)

[Sascha Silbe]

Daniel Drake <dsd at laptop.org> writes:

> On Tue, Jul 3, 2012 at 8:04 AM, Sascha Silbe <silbe at activitycentral.com> wrote:
>> It seems the if/elif blocks and the else block now handle cases that are
>> different in nature. While if block vs. elif block distinguishes between
>> the versions of Telepathy in use, if/elif blocks vs. else block
>> seems to distinguish between different stages of the protocol. That's
>> fine, but a comment explaining the latter check would make this code
>> block easier to understand.
>
> I don't think its related to different stages in the protocol, but I
> don't know a huge amount on this topic. Can you point me towards what
> makes you think this so that I can write a good comment?

It's just what it looked like to me. If it isn't, a comment would be
even more important. So if you can figure out why we're doing this,
please add a corresponding note in the code. If you can't, please add a
FIXME. Code that we don't understand is likely to cause trouble in the
future.


>> This part seems to be a direct copy of the corresponding block in
>> __list_properties_cb(). How about factoring it out into a class
>> constant?
>
> It's not a direct copy - the names are different, as are the set of
> properties (mentioned in commit message).

You're right, I didn't look carefully enough.


> I'll address the other comments, thanks.

Thanks! I expect your next patch to be good enough, so here's my

Acked-by: Sascha Silbe <silbe at activitycentral.com>

However, please send the final patch to the list before pushing.

Sascha
-- 
http://sascha.silbe.org/
http://www.infra-silbe.de/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 489 bytes
Desc: not available
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20120703/97155581/attachment.pgp>