[GitHub] [zookeeper] symat commented on issue #1131: ZOOKEEPER-3599: cli.c: Introduce (optional) 'getopt' option parsing

[GitBox <gi...@apache.org>]

symat commented on issue #1131: ZOOKEEPER-3599: cli.c: Introduce (optional) 'getopt' option parsing
URL: https://github.com/apache/zookeeper/pull/1131#issuecomment-557119967
 
 
   Sorry, I didn't see this PR before.
   
   I actually much prefer the `getopt` way, as it provides more standard unix-like and extendable argument handling. Using the header-only implementation for windows keeps the code easier to understand and maintain (less `ifdef` in the code). Also I think it is a good idea if the user can use the same CLI syntax on all platforms.
   
   In general I prefer backward compatible changes. In the case of the C command line client, I am not that sure. I don't think people would use it heavily, especially not as an interface. But maybe you are right and someone is using / calling it from some devops or monitoring tool. In this case it would be nice to keep the old argument parsing.
   
   If you have the time to work on this and make command line arguments backward compatible, then I would suggest to add it back only as a secondary fallback option. Like if the new `getopt` argument parsing fails (e.g. because the new mandatory server url parameter is missing) then we can try to parse the command line in the old way. But I wouldn't extend the old command line arguments anymore (e.g. I wouldn't add the SSL or debug options for the old command line parsing). I would also add some comments to the code, to explain why we accept two syntaxes.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services