Re: [PATCH 1/2] usertools: use argparse module to get input parameter

[Bruce Richardson <bruce.richardson@intel.com>]

On Mon, Jan 09, 2023 at 08:26:37PM +0800, lihuisong (C) wrote:
> 
> 在 2023/1/9 17:14, Bruce Richardson 写道:
> > On Mon, Jan 09, 2023 at 02:55:46PM +0800, Huisong Li wrote:
> > > The telemetry client script uses argparse module to get input parameter.
> > > 
> > > Signed-off-by: Huisong Li <lihuisong@huawei.com>
> > > ---
> > >   usertools/dpdk-telemetry-client.py | 14 +++++++-------
> > >   1 file changed, 7 insertions(+), 7 deletions(-)
> > > 
> > This is an old script using the older telemetry V1 interface, so I'd
> > generally recommend users switch to using scripts for the v2 interface.
> > That said, no reason not to improve the script while we have it.
> Yes. After all, the telemetry v1 interface and this script are still exist.
> > 
> > > diff --git a/usertools/dpdk-telemetry-client.py b/usertools/dpdk-telemetry-client.py
> > > index df41d04fbe..fd69955b32 100755
> > > --- a/usertools/dpdk-telemetry-client.py
> > > +++ b/usertools/dpdk-telemetry-client.py
> > > @@ -6,6 +6,7 @@
> > >   import os
> > >   import sys
> > >   import time
> > > +import argparse
> > >   BUFFER_SIZE = 200000
> > > @@ -115,13 +116,12 @@ def interactiveMenu(self, sleep_time): # Creates Interactive menu within the scr
> > >   if __name__ == "__main__":
> > >       sleep_time = 1
> > > -    file_path = ""
> > > -    if len(sys.argv) == 2:
> > > -        file_path = sys.argv[1]
> > > -    else:
> > > -        print("Warning - No filepath passed, using default (" + DEFAULT_FP + ").")
> > > -        file_path = DEFAULT_FP
> > > +    parser = argparse.ArgumentParser()
> > > +    parser.add_argument('-s', '--sock_path', default=DEFAULT_FP,
> > > +                        help='Provide socket file path connected by legacy client')
> > > +    args = parser.parse_args()
> > > +
> > While I like using argparse rather than handling args directly, this breaks
> > compatibility.  For anyone already using this script via automation, this
> > would break things, as the path needs to be provided via a "-s" parameter,
> > rather than just tacked on as argv[1].
> If there isn't the modification patch 2/2 mentioned, this script cannot be
> directly used in most scenarios. From the first commit of this script, it's
> just used as a demo client example. See
> commit d1b94da4a4e0 ("usertools: add client script for telemetry")
> From this point of view, can this compatibility issue be ignored?

Agree with you that patch 2/2 is necessary to make script useful for most
cases, and also agree that argparse is the better way to do argument
handling. However, I also think that we can keep compatibility in this
matter - you can add optional positional arguments in argparse to support
backward compatibility.

	parser.add_argument('sock_path', nargs='?', default=...)

should work, I believe, for this case.

/Bruce