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

  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(-)
  

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.

> 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].

>      client = Client()
> -    client.getFilepath(file_path)
> +    client.getFilepath(args.sock_path)
>      client.register()
>      client.interactiveMenu(sleep_time)
> -- 
> 2.22.0
>
  

在 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?
>
>>       client = Client()
>> -    client.getFilepath(file_path)
>> +    client.getFilepath(args.sock_path)
>>       client.register()
>>       client.interactiveMenu(sleep_time)
>> -- 
>> 2.22.0
>>
> .
  

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
  

在 2023/1/9 22:32, Bruce Richardson 写道:
> 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.
It works well. Thank you for your suggestion. I will fix it in v2.