On Wed, Oct 13, 2010 at 6:01 AM, Dipankar Patro <dipan...@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 _______________________________________________ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel