[Reportbug-maint] Bug#458735: reportbug: Please offer option to show bug mbox in mailer

[Sandro Tosi]

Hi Rafael,

On Wed, Dec 15, 2010 at 04:18, Rafael Cunha de Almeida
<rafael-debianbugs at kontesti.me> 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
>> <rafael at kontesti.me> 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