[Sugar-devel] Great Response was Re: [PATCH sugar] Removed hardcoded server url (SL #1976)

[David Farning]

On Wed, Oct 13, 2010 at 6:01 AM, Dipankar Patro <dipankar at seeta.in> wrote:
> Thanks for reviewing it, James.
>
>>    if have_ofw_tree():
>>        sn = read_ofw('mfg-data/SN')
>>        uuid_ = read_ofw('mfg-data/U#')
>>        sn = sn or 'SHF00000000'
>>        uuid_ = uuid_ or '00000000-0000-0000-0000-000000000000'
>>
>> >      else:
>> >          sn = generate_serial_number()
>> >          uuid_ = str(uuid.uuid1())
>> > -        setting_name = '/desktop/sugar/collaboration/jabber_server'
>> > -        jabber_server = client.get_string(setting_name)
>> > -        store_identifiers(sn, uuid_, jabber_server)
>> > +
>> > +    setting_name = '/desktop/sugar/collaboration/jabber_server'
>> > +    jabber_server = client.get_string(setting_name)
>> > +    store_identifiers(sn, uuid_, jabber_server)
>> > +
>> > +    if jabber_server:
>> >          url = 'http://' + jabber_server + ':8080/'
>>
>> You are effectively repeating the previous if statement but using the
>> output ... seems a bit obscure.
>
>  ^^ Before applying this : The url (url for registration) was set from gconf
> only for non-XO devices.
> I moved that outside the first if..else you have quoted, so that on all
> devices the url is taken from the gconf property
> (/desktop/sugar/collaboration/jabber_server)
>
> the second 'if jabber_server:' is put there to check whether the
> jabber_server retrieved from the gcnof property is empty or unset. If
> jabber_server is a valid one, then change url, other wise keep the url =
> initialized one.
>
> I actually went with the above modification due to the chain of mails here :
> http://lists.sugarlabs.org/archive/sugar-devel/2010-July/025265.html
>
> Regards,
> Dipankar

Great response! You clearly explained what you did, _why_ you did it,
and how you came to that decision.

Now a reviewer can effectively make a decision of 'that makes sense,
ACK' or 'what if you look at it this way?'

david