Re: mailgw argument parsing

On Mon, Nov 21, 2022 at 04:08:29PM -0500, John P. Rouillard wrote:
> Hi Ralf:
> 
> In message <[email protected]>,
> Ralf Schlatterbeck writes:
> >I'm in the process of adding OAUTH support to roundup-mailgw.
> 
> Neat. Do you know about: https://issues.roundup-tracker.org/issue2551239?

No, I didn't, this is related but not immediately my target :-)

> >Now the current argument parsing in mailgw uses getopt.getopt for
> >argument parsing. I'd like to move this to argparse.ArgumentParser when
> >at it (I need to add at least one new option).
> 
> I have no problem with that. I think argparse worked pretty much the
> same in Python2.
> 
> We have an open ticket on replacing optparse with argparse:
> 
>      https://issues.roundup-tracker.org/issue2551195
> 
> standardizing on argparse would be good. Drop any hints on using
> argparse in the ticket.

I'll look into this, should be easy enough. But it looks also like these
scripts are neither used nor tested?

Note that the current implementation of roundup_mailgw.py uses an even
older method of parsing arguments (but probably not deprecated).

> 
> >There is a -c option to set the Class we're acting on, defaulting to
> >MAIL_DEFAULT_CLASS (in the config-file default_class in section [mailgw])
> >
> >The -C and -S options to mailgw are used to set properties on a class,
> >-C accepts the values 'file', 'issue', 'msg', 'user' and defaults to 'msg'.
> >Note that even if a -c option is present, setting the default class to
> >something other than 'issue' the -C option will insist on 'issue'. At
> >least that's how I understand the code.
> 
> Hmm... that seems wrong. -C is usually msg., but I have never used
> '-c' so.... that's possible.

Say you have your issue-class named 'it_issue' (I do have several
trackers where this is the case, see https://github.com/time-track-tool)
then you would need to use

-c it_issue -C issue -S ...

I think I've been bitten by this once but I currently do not have any
use-cases where I'm setting issue properties in the mailgw.

> 
> My use case was to categorize messages into "reply" and "comment"
> types arriving on different email addresses.  The two addresses
> notified different nosy lists. So:
> 
>    -C msg -S "messagetype=reply" (notifies nosy and verynosy list)
>    -C msg -S "messagetype=comment"  (notifies only on the verynosy list)
> 
> I was using the issue class in both cases.

Nice.

> I'm not actually sure when you would use -c to change the class to
> "file", "msg" etc. I could see -c "bugs" -c "enhancement" for
> different classes of issues. But I am not sure how using "user" would
> work.  I think the mailgw requires something with the shape of an
> issue with messages and attachments/files??

Yes, the idea is that you have different issue classes (see above).
I'm running a tracker that has an 'issue' class and a 'support' class
with two different email addresses.

A use-case for 'user' might be to do some last-use statistics on user.
But I've never seen that use-case in the wild :-)

> 
> >So to set several properties on e.g. issue and user you would use
> >
> >-C issue -S issueprop1=value1 -S issueprop2=value2 -C user -S userprop=uvalue
> >
> >This interleaving of -C and -S options is not only un-intuitive for
> >users (well, at least for me)
> 
> Agreed, not intuitive but the man page only allows one pair of -C/-S
> where -C is optional:
> 
>   ... [[-C  class] -S field=value] ...
> 
> If it parses multiple pairs, I am going to claim that is a bug.

No, I think the example above is explicitly allowed. You *could* use
semicolon to pack several -S options into one but it is equally possible
to use several -S with one -C, I think (untested).

> I suppose you could do something like
> 
>   -C file -S visibility=hidden
> 
> to make all  attached files via that email address hidden or
> 
>   -c msg -S messagetype=reply -C issue -S visibility=hidden
> 
> when you tell people to send security issues to
> 
>   [email protected]
> 
> to make the issue invisible when created.

Yes, nice examples, should go into the docs :-)

> >but is fundamentally incompatible with how ArgumentParser works.
> 
> I have only taken a glance at argparse (when I opened the issue) and
> thought multiple flags could be parsed into a list using append. But
> in this case supporting multiple pairs of -C/-S would require a -C
> before the second -S. Then you just zip the lists together. But this
> does make -C manadatory.

Yes, I thought so, but once you want to allow multiple -S with a single
-C this fails.

> >So I'd suggest we change this to only a -S (--set-value) option that
> >optionally takes a classname before a '.', the above would become:
> >
> >-S issue.issueprop1=value1 -S issue.issueprop2=value2 -S user.userprop=uvalue
> 
> How about still supporting -C msg so:
> 
>  -C file -S fileprop1=value1 -S issue.issueprop2=value2 -S user.userprop=uvalue
> 
> provides the default if no class is specified.

We cannot know for which -S options the default applies if you want to
allow multiple -C options.

> 
> So in this case it would be the same as if the first -S argument was:
> 
>    file.fileprop1=value1
> 
> and the default is still msg if -C isn't used.
> 
> The advantage is that existing invocations don't change and you get to
> set multiple things from the email address.

See above: Currently we allow multiple -C options and these *will* fail.

I'm already quite far with the implementation, I think I'm going to push
that on a branch and we can discuss/test this further? I currently still
think getting rid of -C is more intuitive. And an invocation that
actually uses -C will fail with an error message, so no harm done if we
put that into the 'upgrading' docs? And since we're leaving the current
default of 'msg' an invocation using only -S will stay compatible.

Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   www.runtux.com
Reichergasse 131, A-3411 Weidling       email: [email protected]