[Sugar-devel] [PATCH sugar-toolkit] presence: use RoomConfig1 to configure channel properties (#3629)
silbe at activitycentral.com
Tue Jul 3 17:10:50 EDT 2012
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
>> This part seems to be a direct copy of the corresponding block in
>> __list_properties_cb(). How about factoring it out into a class
> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Size: 489 bytes
Desc: not available
More information about the Sugar-devel