Tomeu Vizoso wrote: > On Wed, Mar 19, 2008 at 5:05 PM, Simon Schampijer <[EMAIL PROTECTED]> wrote: >> Hi Tomeu, Eben, >> >> I went through the last commits until 'Remove activities.defaults, not >> used any more'. I will go through the rest as well. Here are my thoughts: >> >> *** Pulse activity icons in the ring during launch >> commit 3670fda59103c32fcbe8b28a9f8e77a75ac15d31 >> >> pylint: >> - src/model/homemodel.py: Unused import dbus (maybe you can remove >> that when you fix something in that file) > > Done > >> - 139: Line too long (81/80) >> - 198: Line too long (81/80) > > Do we care about one char more?
If we do it we should do it right i guess :) >> _get_color() could be a function since it does not access the actual >> object. Probably ok in this case. > > I like to group code in classes when it makes sense only in that > class. Everybody agree? A later commit makes the method use > self._level, though. Yup it makes sense to group that there, having in the scope of the module would be wrong as well. >> W:211:ActivityIcon: Use of "property" on an old style class >> W:215:ActivityIcon: Use of "property" on an old style class > > False positive from pylint, ActivityIcon is a new style class as > inherits from CanvasIcon. hmm, again. >> Why do we 'wrap' the get_version method here with the property construct? > > Because the code is shorter and leaves more room for in the future > changing the implementation without breaking API compatibility. I > think we want to do this in general? Not expose setters nor getters > but properties? Agreed as well understood this better when we discussed the custom setter getter case today. >> *** Made background colors of zoom spheres white, for consistency >> commit c5b776bcf27b4f6e638fac3658437a3c194e7ddd >> - src/view/BuddyIcon.py >> - no semicolon > > Done > >> *** Fixed scale and placement of a single icon in the ring >> commit ef75cabea665c436fc40cee887462931462d6bbf >> - what do the '**' instead of '*' mean ? > > It dereferences twice. It's some pygtk magic for getting the rest of > the named args as a dict. Oh ok, makes sense. >> *** Store favorite activities in the profile dir. >> commit 3b5131e4b16eb5a83278de3787a5b1f4763ae566 >> - no need to import os.path as well we import os already > > Done > >> *** General Errors >> src.view.home.activitiesring: >> E: 75:ActivitiesRing.__activity_added_cb: Undefined variable 'info' >> - should probably be activity_info > > Done > >> src.view.home.activitieslist >> W: 20: Unused import gtk > > Done > >> * General Thoughts >> What is the advantage from simplejson over json. The home page says: >> simplejson is API compatible with json-py, but is more easily >> extensible, has better unicode support, is probably also faster, and is >> MIT licensed (json-py is LGPL/GPL). Should we recommend people using >> simplejson? > > My main problem with json is that it requires the data types to be the > base types in python. So if you want to encode data from dbus, you'll > have to convert all dbus.String to str. So perhaps we should go always > with simplejson? Makes sense to me. Simon _______________________________________________ Sugar mailing list [email protected] http://lists.laptop.org/listinfo/sugar
