Regarding the lumping in one file, I'm starting to like the separate files more. My reasoning for lumping all dialogs together in 'olivedialogs' was: When I first eyeballed the code I found the naming used not very logical. Names like add.py, mkdir.py do not indicate that the files are for dialogs. Also, menu.py is only the contextmenu.. not the menu of the main window. A file called olivedialogs at least indicates what's inside. But renaming add.py to adddialog.py with an AddDialog class inside may be even better still. But as PEP8 says: Keep module names short. So I guess keeping add.py is good. Or 1 dialogs.py file since it will be loaded as bzrlib.plugins.gtk.olive.dialogs? Szilveszter, what is your ruling?
I agree with redoing this. Likely sending a bundle for each dialog separately. Moving all to a different file can be done afterwards if wanted.
On a sidenote, putting the bulk of the code in __init__.py was a nasty surprise. It took me some time to notice the size of the file that I ignored at first looking for the olive logic. :P
Speaking of PEP8.. it also states that all imports should be done at the top of the file. At least olive/__init__.py has imports all over the place. Is this intentional so to only load certain modules when needed?
Jasper Jelmer Vernooij wrote:
Jelmer Vernooij has voted abstain. Status is now: Waiting Comment:I'm not sure lumping all of the dialogs in a single file is a good idea and if it is, I'd rather see that being done in a separate merge. There are also a few formatting issues (not PEP8). I'll let Szilveszter do the proper review of this since this is originally his code.For details, see: http://bundlebuggy.aaronbentley.com/request/%3C487BA8D3.30308%40xs4all.nl%3E
signature.asc
Description: OpenPGP digital signature
-- bzr-gtk mailing list [email protected] Modify settings or unsubscribe at: https://lists.canonical.com/mailman/listinfo/bzr-gtk
