Hi Rafael, On Wed, Dec 15, 2010 at 04:18, Rafael Cunha de Almeida <[email protected]> wrote: > On Sat, Dec 11, 2010 at 11:19:20PM +0100, Sandro Tosi wrote: >> Hi Rafael, >> first of all, let me thank you for working on reportbug bugs! > > Hello, > > No problem. This is what free software is all about, right? :-)
exactly! :) >> On Sun, Dec 5, 2010 at 17:32, Rafael Cunha de Almeida >> <[email protected]> wrote: >> > On Wed, Jan 02, 2008 at 05:59:15AM -0800, Josh Triplett wrote: >> >> Package: reportbug >> >> Version: 3.39 >> >> Severity: wishlist >> >> >> >> Using bts from devscripts, "bts show --mbox $BUGNUM" will download the >> >> mbox for $BUGNUM and show it in a mailer, defaulting to mutt ("mutt -f >> >> %s"). I'd love to have the same feature in querybts and reportbug, so >> >> that when viewing a bug, I could press a key (such as 'm', which looks >> >> available) to see the mbox. >> > >> > I wrote a patch that implements that feature. I tried to follow the same >> > style of the rest of the code, but if anyone thinks the patch is not >> > good enough, just talk to me and I will change it as needed. >> >> These are my impressions, just looking at the patch (so still even trying >> them): > > Alright. You raised good points. Hopefully I made the patch better this > time around. A lot better, so much that I just merged it: thanks & congrats! Anyhow I'm commenting your reply, just in case >> - I don't like the add of a submodule just to handle the mbox launcher >> method: reportbug.utils seems more than enough > > I thought about adding it to utils, but then I thought that maybe it's > getting too overloaded with functions. Perhaps, it would be good to > start moving things away from there. Maybe, in the future, make utils > a package with different modules inside. Just an idea. What do you > think? that's a good idea, but maybe now it's not a good time > Another point is that importing utils inside of text_ui creates a > circular module dependency: utils imports ui_text which, in turn, > imports utils. In this new patch I have moved launch_mbox_reader to > utils.py and I made it work even with the circular dependency. But I > don't like this solution too much. yeah, it sucks! there is already a circular depends (IIRC between utils and debianbts), and someday I'll have to find a way to break it. >> - don't add 2 different options between querybts and reportbug: if >> reportbug doesn't have '-e' free, just use the long option > > People could try using -e in querybts meaning something else, right? > Good thinking! I removed -e from querybts, I think just the long option > is enough. indeed >> - I'm not sure I want this feature also in reportbug: just in querybts? > > Hm. What's the problem in having it in reportbug? Launch browser is > already there, why would an user want to see the full report using a > browser but not using an email client? If someone is going to report a > bug and he finds out the bug is already reported, then he'd probably > be curious to read all the answers (and possible find a workaround or > at least some insight.) > > I left it in reportbug still. It should be pretty simple to remove it > anyway. oooook, let's go with both :) >> - you call it everywhere mbox_something, so even use '-e' command-line >> option, and also as an option inside reportbug seems wrong: let's try >> to recall mbox somehow ('x'?). > > The problem is that all letters of ``mbox'' word are taken, so I thought > about `e' as in e-mail. `x' is taken as well, when you enter a bug > report `x' means `Provide extra information'. I don't know, perhaps `e' > is as good as anything else :P yeah, no luck, 'e' it's fine then. >> - 'e' : 'Launch e-mail client to read full log.' and 'e' : 'Open a >> specific report using the specified mbox reader.' - try use the same >> message consistently (and the second one doesn't seem in english :) ). > > hehe not being a native English speaker, I gotta say that I don't even > know what's wrong with the second one :P. However, I think I made it > better. > >> - I don't like that much validation check on bug number in the text_ui >> code, better implement it in the code that's going to fetch the data >> (so you'll have it once, and not cut&paste twice). > > Yes, I thought that too. But then I saw the same sort of thing going on > in other options such as 'y' and 'm' (where I copied that part of code > from), so I thought I'd stick to the style. For now I added a function, > called _launch_mbox_reader for handling those cases inside the view. > After all, it's specific to each UI how they handle the list of bugs, in > case of the text UI it has to check if the user didn't type anything > wrong, but in GTK the user can't really screw up. > > What I'd really like to do is create a controller class to handle all > those optins and have it to just be called by the view. I think it would > make the code for text ui cleaner. What are your thoughts on the > subject? yeah, there's a lot of places where refactoring would help, and I think I'll dedicate some time to it, only there's more pressing stuff to do with reportbug (complete unittests suite, convert to use SOAP, etc) than that. Cheers & thanks again for your work, -- Sandro Tosi (aka morph, morpheus, matrixhasu) My website: http://matrixhasu.altervista.org/ Me at Debian: http://wiki.debian.org/SandroTosi _______________________________________________ Reportbug-maint mailing list [email protected] http://lists.alioth.debian.org/mailman/listinfo/reportbug-maint
