Am Donnerstag, den 17.07.2008, 21:58 +0200 schrieb Jasper Groenewegen: > With regard to the formatting issues.. I looked at PEP8 and I see that I > at least should look at the line lengths and docstrings. What else is in > olivedialogs.py that bothers you, Jelmer? The particular thing I noticed was that there aren't two lines of whitespace in between top-level items (such as classes). Also, there are far more empty lines in regular code than appears necessary for the readability.
The main issue I have with lumping everything in one file in this case is that it makes it harder to see the actual changes. I have no objection against making that sort of big changes, but I'd rather see it happen in a different patch so those changes can be reviewed independently. I'm not opposed to combining files in general if the various files themselves are short. > 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 :-) Yeah, I agree. > 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? We try to do imports at the top of the file when writing new code but old code may not always be written that way. Using imports later inside of the file is sometimes also unavoidable when there are circular imports. HTH, Cheers, Jelmer > 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 > > -- Jelmer Vernooij <[EMAIL PROTECTED]> - http://samba.org/~jelmer/ Jabber: [EMAIL PROTECTED] -- bzr-gtk mailing list [email protected] Modify settings or unsubscribe at: https://lists.canonical.com/mailman/listinfo/bzr-gtk
