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? > _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. > 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. > 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? > *** 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. > *** 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? Thanks a lot for the review, Tomeu _______________________________________________ Sugar mailing list [email protected] http://lists.laptop.org/listinfo/sugar
