argument parsing refactoring round3

[David Bremner]

David Bremner  writes:

> I think a dealt with all of Mark's comments, even "notmuch help
> --help".
>

Pushed the amended version of these patches, as some small step to
getting the revision tracking patches merged.

d

Re: argument parsing refactoring round3

[David Bremner]

David Bremner da...@tethera.net writes:

 I think a dealt with all of Mark's comments, even notmuch help
 --help.


Pushed the amended version of these patches, as some small step to
getting the revision tracking patches merged.

d
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

argument parsing refactoring round3

[Tomi Ollila]

On Sat, Apr 11 2015, David Bremner  wrote:

> David Bremner  writes:
>
>> guyzmo  writes:
>>
>>> I was wondering whether you had considered actually using `docopt` to
>>> generate the CLI parser from the output.
>
>> As it stands, I'm not particularly annoyed with the notmuch argument
>> parsing code, so I mainly see negative issues about your proposal.
>
> On the other hand, I do find the config handling code annoying, so if
> people have some ideas about that, I am interested to hear them.  There
> is now almost 1000 lines of C in notmuch-config.c, and despite some
> efforts to streamline things, there is a lot of copying and pasting
> every time someone wants to add a configuration option.

I reviewed the changes -- last ones 'cursory' as I think those don't 
break any "real" functionality -- and if help works bad then one can
always resort to testing and reading code >;)...

...but I did not test -- automated tests (to ensure it doesn't break
anything be handy... more complete would limit anyone's interest to
implement alternate option handling (even further).

As much I also liked "better" option handling code new code (vast of that)
would also require significant reviewing effort which no-one will like
to do >;/

With that said, +1 from me to the config handling changes. 
(or should I give it some hand-testing (how?)

Tomi

>
> d

Re: argument parsing refactoring round3

[Tomi Ollila]

On Sat, Apr 11 2015, David Bremner da...@tethera.net wrote:

 David Bremner da...@tethera.net writes:

 guyzmo guyzmo+notm...@m0g.net writes:

 I was wondering whether you had considered actually using `docopt` to
 generate the CLI parser from the output.

 As it stands, I'm not particularly annoyed with the notmuch argument
 parsing code, so I mainly see negative issues about your proposal.

 On the other hand, I do find the config handling code annoying, so if
 people have some ideas about that, I am interested to hear them.  There
 is now almost 1000 lines of C in notmuch-config.c, and despite some
 efforts to streamline things, there is a lot of copying and pasting
 every time someone wants to add a configuration option.

I reviewed the changes -- last ones 'cursory' as I think those don't 
break any real functionality -- and if help works bad then one can
always resort to testing and reading code ;)...

...but I did not test -- automated tests (to ensure it doesn't break
anything be handy... more complete would limit anyone's interest to
implement alternate option handling (even further).

As much I also liked better option handling code new code (vast of that)
would also require significant reviewing effort which no-one will like
to do ;/

With that said, +1 from me to the config handling changes. 
(or should I give it some hand-testing (how?)

Tomi


 d
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

argument parsing refactoring round3

[David Bremner]

David Bremner  writes:

> guyzmo  writes:
>
>> I was wondering whether you had considered actually using `docopt` to
>> generate the CLI parser from the output.

> As it stands, I'm not particularly annoyed with the notmuch argument
> parsing code, so I mainly see negative issues about your proposal.

On the other hand, I do find the config handling code annoying, so if
people have some ideas about that, I am interested to hear them.  There
is now almost 1000 lines of C in notmuch-config.c, and despite some
efforts to streamline things, there is a lot of copying and pasting
every time someone wants to add a configuration option.

d

Re: argument parsing refactoring round3

[David Bremner]

David Bremner da...@tethera.net writes:

 guyzmo guyzmo+notm...@m0g.net writes:

 I was wondering whether you had considered actually using `docopt` to
 generate the CLI parser from the output.

 As it stands, I'm not particularly annoyed with the notmuch argument
 parsing code, so I mainly see negative issues about your proposal.

On the other hand, I do find the config handling code annoying, so if
people have some ideas about that, I am interested to hear them.  There
is now almost 1000 lines of C in notmuch-config.c, and despite some
efforts to streamline things, there is a lot of copying and pasting
every time someone wants to add a configuration option.

d
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

argument parsing refactoring round3

[David Bremner]

guyzmo  writes:

> Hi David,

[...]

> I see you patching and repatching notmuch's CLI to improve it, and I was
> wondering whether you had considered actually using `docopt` to generate
> the CLI parser from the output.
>
> It's possible to chain docopts to create a CLI UI very much alike the
> git command, and it's more easily maintainable, as you're actually
> generating the code from the `--help` page instead of the other way
> around, making you focus on how you want the user to use the CLI only.

[...]

> what do you believe?

It's an interesting idea, and if I was faced with writing CLI parser
from scratch (i.e. 4 years ago) I would investigate it more seriously.
As it stands, I'm not particularly annoyed with the notmuch argument
parsing code, so I mainly see negative issues about your proposal.

- I'm not sure how much this change would ripple through the rest of the
  notmuch code. At least all of the variables set by the current
  argument parsing code would have to be set foo=args.foo, or replaced
  everywhere with args.foo.

- It would require modifying the notmuch CLI to conform to the
  conventions of docopt. Of course, you might consider this a feature,
  but I think as many people would be annoyed as would be happy.

- The most dramatic example of an appartently missing feature from
  docopt is keyword arguments of the form

--output=(messages|threads|summary).

  These are the reason we rolled our own parser in the first place,
  rather than using e.g. gnu getopt.  docopt says it doesn't do data
  validation, which is fine philosophically, but by the time we add back
  in validation code, I'm not sure we win very much in the
  maintainability department.

- I don't really know about the health of the docopt.c project, compared
  to the python version. It seems somewhat unfinished [1]; in particular
  a lack of positional arguments seems like a showstopper for us.
  arguments. There has never been a release (which is the norm for
  github, but not for dependencies of notmuch), and the last commit was
  in December of 2014.

- Since docopt_c_py is not widely in distros (it isn't in Debian, for
  example), we'd have to emded it the notmuch source.  It's only 217
  lines of fairly simple python, but embedding 3rd party code is
  something we try pretty hard to avoid.

As always, my lack of enthusiasm doesn't prevent someone else from
investigating further, but hopefully the points I listed above give
anyone doing such investigation some hints as to what I (and I suspect
not just I) would object to about any hypothetical patches.

David

[1]: From the README.md "Note, at this point the code generator handles
only options (positional arguments, commands and pattern matching will
follow)."

argument parsing refactoring round3

[guyzmo]

Hi David,

On Wed, Apr 08, 2015 at 04:30:38AM +0900, David Bremner wrote:
> I think a dealt with all of Mark's comments, even "notmuch help
> --help".
> 
> I ended up creating a new function for the places where we want to
> process _only_ the shared options (config, setup, and help)

I see you patching and repatching notmuch's CLI to improve it, and I was
wondering whether you had considered actually using `docopt` to generate
the CLI parser from the output.

It's possible to chain docopts to create a CLI UI very much alike the
git command, and it's more easily maintainable, as you're actually
generating the code from the `--help` page instead of the other way
around, making you focus on how you want the user to use the CLI only.

Here's the link to the C parser generator:

https://github.com/docopt/docopt.c

it might not be perfect as is, but it could be worth trying out? I
actually never tried the .c version of docopt.

I had a more extensive experience with the python version, and since
then I totally dropped argparse.

https://github.com/docopt/docopt.c

I even tend to believe that one could create a full CLI using
python+docopt to actually control notmuch using the python notmuch
interface… Even though I'm pretty sure some would yell at me and
want to burn me as an heretic just for suggesting that :-)

what do you believe?

-- 
Guyzmo

argument parsing refactoring round3

[David Bremner]

I think a dealt with all of Mark's comments, even "notmuch help
--help".

I ended up creating a new function for the places where we want to
process _only_ the shared options (config, setup, and help)

diff --git a/notmuch-client.h b/notmuch-client.h
index ab0d188..78680aa 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -467,5 +467,7 @@ notmuch_database_dump (notmuch_database_t *notmuch,

 #include "command-line-arguments.h"
 extern const notmuch_opt_desc_t  notmuch_shared_options [];
-void notmuch_process_shared_options (const char* help_name);
+void notmuch_process_shared_options (const char* subcommand_name);
+int notmuch_minimal_options (const char* subcommand_name,
+int argc, char **argv);
 #endif
diff --git a/notmuch-config.c b/notmuch-config.c
index f2cd6a8..9348278 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -874,17 +874,10 @@ notmuch_config_command (notmuch_config_t *config, int 
argc, char *argv[])
 int ret;
 int opt_index;

-notmuch_opt_desc_t options[] = {
-   { NOTMUCH_OPT_INHERIT, (void *) _shared_options, NULL, 0, 0 },
-   { 0, 0, 0, 0, 0 }
-};
-
-opt_index = parse_arguments (argc, argv, options, 1);
+opt_index = notmuch_minimal_options ("config", argc, argv);
 if (opt_index < 0)
return EXIT_FAILURE;

-notmuch_process_shared_options (argv[0]);
-
 /* skip at least subcommand argument */
 argc-= opt_index;
 argv+= opt_index;
diff --git a/notmuch-search.c b/notmuch-search.c
index 994ae58..b89a17e 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -740,6 +740,7 @@ notmuch_address_command (notmuch_config_t *config, int 
argc, char *argv[])
  { "false", NOTMUCH_EXCLUDE_FALSE },
  { 0, 0 } } },
{ NOTMUCH_OPT_INHERIT, (void *) _options, NULL, 0, 0 },
+   { NOTMUCH_OPT_INHERIT, (void *) _shared_options, NULL, 0, 0 },
{ 0, 0, 0, 0, 0 }
 };

@@ -747,6 +748,8 @@ notmuch_address_command (notmuch_config_t *config, int 
argc, char *argv[])
 if (opt_index < 0)
return EXIT_FAILURE;

+notmuch_process_shared_options (argv[0]);
+
 if (! (ctx->output & (OUTPUT_SENDER | OUTPUT_RECIPIENTS)))
ctx->output |= OUTPUT_SENDER;

diff --git a/notmuch-setup.c b/notmuch-setup.c
index 21d074a..7dd5822 100644
--- a/notmuch-setup.c
+++ b/notmuch-setup.c
@@ -133,7 +133,6 @@ notmuch_setup_command (notmuch_config_t *config,
 size_t new_tags_len;
 const char **search_exclude_tags;
 size_t search_exclude_tags_len;
-int opt_index;

 #define prompt(format, ...)\
 do {   \
@@ -146,18 +145,9 @@ notmuch_setup_command (notmuch_config_t *config,
chomp_newline (response);   \
 } while (0)

-notmuch_opt_desc_t options[] = {
-   { NOTMUCH_OPT_INHERIT, (void *) _shared_options, NULL, 0, 0 },
-   { 0, 0, 0, 0, 0 }
-};
-
-opt_index = parse_arguments (argc, argv, options, 1);
-if (opt_index < 0)
+if (notmuch_minimal_options ("setup", argc, argv) < 0)
return EXIT_FAILURE;

-/* We can't use argv here as it is sometimes NULL */
-notmuch_process_shared_options ("setup");
-
 if (notmuch_config_is_new (config))
welcome_message_pre_setup ();

diff --git a/notmuch.c b/notmuch.c
index c7f8c8f..2198b73 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -59,18 +59,40 @@ const notmuch_opt_desc_t notmuch_shared_options [] = {
  * notmuch_process_shared_options (subcommand_name);
  */
 void
-notmuch_process_shared_options (const char *help_name) {
+notmuch_process_shared_options (const char *subcommand_name) {
 if (print_version) {
printf ("notmuch " STRINGIFY(NOTMUCH_VERSION) "\n");
exit (EXIT_SUCCESS);
 }

 if (print_help) {
-   int ret = _help_for (help_name);
+   int ret = _help_for (subcommand_name);
exit (ret);
 }
 }

+/* This is suitable for subcommands that do not actually open the
+ * database.
+ */
+int notmuch_minimal_options (const char *subcommand_name,
+ int argc, char **argv)
+{
+int opt_index;
+
+notmuch_opt_desc_t options[] = {
+   { NOTMUCH_OPT_INHERIT, (void *) _shared_options, NULL, 0, 0 },
+   { 0, 0, 0, 0, 0 }
+};
+
+opt_index = parse_arguments (argc, argv, options, 1);
+
+if (opt_index < 0)
+   return -1;
+
+/* We can't use argv here as it is sometimes NULL */
+notmuch_process_shared_options (subcommand_name);
+return opt_index;
+}

 static command_t commands[] = {
 { NULL, notmuch_command, TRUE,
@@ -250,7 +272,15 @@ _help_for (const char *topic_name)
 static int
 notmuch_help_command (unused (notmuch_config_t * config), int argc, char 
*argv[])
 {
-argc--; argv++; /* Ignore "help" */
+int opt_index;
+
+opt_index = notmuch_minimal_options ("help", argc, argv);
+if (opt_index < 

Re: argument parsing refactoring round3

[David Bremner]

guyzmo guyzmo+notm...@m0g.net writes:

 Hi David,

[...]

 I see you patching and repatching notmuch's CLI to improve it, and I was
 wondering whether you had considered actually using `docopt` to generate
 the CLI parser from the output.

 It's possible to chain docopts to create a CLI UI very much alike the
 git command, and it's more easily maintainable, as you're actually
 generating the code from the `--help` page instead of the other way
 around, making you focus on how you want the user to use the CLI only.

[...]

 what do you believe?

It's an interesting idea, and if I was faced with writing CLI parser
from scratch (i.e. 4 years ago) I would investigate it more seriously.
As it stands, I'm not particularly annoyed with the notmuch argument
parsing code, so I mainly see negative issues about your proposal.

- I'm not sure how much this change would ripple through the rest of the
  notmuch code. At least all of the variables set by the current
  argument parsing code would have to be set foo=args.foo, or replaced
  everywhere with args.foo.
   
- It would require modifying the notmuch CLI to conform to the
  conventions of docopt. Of course, you might consider this a feature,
  but I think as many people would be annoyed as would be happy.

- The most dramatic example of an appartently missing feature from
  docopt is keyword arguments of the form
  
--output=(messages|threads|summary).

  These are the reason we rolled our own parser in the first place,
  rather than using e.g. gnu getopt.  docopt says it doesn't do data
  validation, which is fine philosophically, but by the time we add back
  in validation code, I'm not sure we win very much in the
  maintainability department.
  
- I don't really know about the health of the docopt.c project, compared
  to the python version. It seems somewhat unfinished [1]; in particular
  a lack of positional arguments seems like a showstopper for us.
  arguments. There has never been a release (which is the norm for
  github, but not for dependencies of notmuch), and the last commit was
  in December of 2014.

- Since docopt_c_py is not widely in distros (it isn't in Debian, for
  example), we'd have to emded it the notmuch source.  It's only 217
  lines of fairly simple python, but embedding 3rd party code is
  something we try pretty hard to avoid.

As always, my lack of enthusiasm doesn't prevent someone else from
investigating further, but hopefully the points I listed above give
anyone doing such investigation some hints as to what I (and I suspect
not just I) would object to about any hypothetical patches.

David

[1]: From the README.md Note, at this point the code generator handles
only options (positional arguments, commands and pattern matching will
follow).
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: argument parsing refactoring round3

[guyzmo]

Hi David,

On Wed, Apr 08, 2015 at 04:30:38AM +0900, David Bremner wrote:
 I think a dealt with all of Mark's comments, even notmuch help
 --help.
 
 I ended up creating a new function for the places where we want to
 process _only_ the shared options (config, setup, and help)

I see you patching and repatching notmuch's CLI to improve it, and I was
wondering whether you had considered actually using `docopt` to generate
the CLI parser from the output.

It's possible to chain docopts to create a CLI UI very much alike the
git command, and it's more easily maintainable, as you're actually
generating the code from the `--help` page instead of the other way
around, making you focus on how you want the user to use the CLI only.

Here's the link to the C parser generator:

https://github.com/docopt/docopt.c

it might not be perfect as is, but it could be worth trying out? I
actually never tried the .c version of docopt.

I had a more extensive experience with the python version, and since
then I totally dropped argparse.

https://github.com/docopt/docopt.c

I even tend to believe that one could create a full CLI using
python+docopt to actually control notmuch using the python notmuch
interface… Even though I'm pretty sure some would yell at me and
want to burn me as an heretic just for suggesting that :-)

what do you believe?

-- 
Guyzmo
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

argument parsing refactoring round3

[David Bremner]

I think a dealt with all of Mark's comments, even notmuch help
--help.

I ended up creating a new function for the places where we want to
process _only_ the shared options (config, setup, and help)

diff --git a/notmuch-client.h b/notmuch-client.h
index ab0d188..78680aa 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -467,5 +467,7 @@ notmuch_database_dump (notmuch_database_t *notmuch,
 
 #include command-line-arguments.h
 extern const notmuch_opt_desc_t  notmuch_shared_options [];
-void notmuch_process_shared_options (const char* help_name);
+void notmuch_process_shared_options (const char* subcommand_name);
+int notmuch_minimal_options (const char* subcommand_name,
+int argc, char **argv);
 #endif
diff --git a/notmuch-config.c b/notmuch-config.c
index f2cd6a8..9348278 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -874,17 +874,10 @@ notmuch_config_command (notmuch_config_t *config, int 
argc, char *argv[])
 int ret;
 int opt_index;
 
-notmuch_opt_desc_t options[] = {
-   { NOTMUCH_OPT_INHERIT, (void *) notmuch_shared_options, NULL, 0, 0 },
-   { 0, 0, 0, 0, 0 }
-};
-
-opt_index = parse_arguments (argc, argv, options, 1);
+opt_index = notmuch_minimal_options (config, argc, argv);
 if (opt_index  0)
return EXIT_FAILURE;
 
-notmuch_process_shared_options (argv[0]);
-
 /* skip at least subcommand argument */
 argc-= opt_index;
 argv+= opt_index;
diff --git a/notmuch-search.c b/notmuch-search.c
index 994ae58..b89a17e 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -740,6 +740,7 @@ notmuch_address_command (notmuch_config_t *config, int 
argc, char *argv[])
  { false, NOTMUCH_EXCLUDE_FALSE },
  { 0, 0 } } },
{ NOTMUCH_OPT_INHERIT, (void *) common_options, NULL, 0, 0 },
+   { NOTMUCH_OPT_INHERIT, (void *) notmuch_shared_options, NULL, 0, 0 },
{ 0, 0, 0, 0, 0 }
 };
 
@@ -747,6 +748,8 @@ notmuch_address_command (notmuch_config_t *config, int 
argc, char *argv[])
 if (opt_index  0)
return EXIT_FAILURE;
 
+notmuch_process_shared_options (argv[0]);
+
 if (! (ctx-output  (OUTPUT_SENDER | OUTPUT_RECIPIENTS)))
ctx-output |= OUTPUT_SENDER;
 
diff --git a/notmuch-setup.c b/notmuch-setup.c
index 21d074a..7dd5822 100644
--- a/notmuch-setup.c
+++ b/notmuch-setup.c
@@ -133,7 +133,6 @@ notmuch_setup_command (notmuch_config_t *config,
 size_t new_tags_len;
 const char **search_exclude_tags;
 size_t search_exclude_tags_len;
-int opt_index;
 
 #define prompt(format, ...)\
 do {   \
@@ -146,18 +145,9 @@ notmuch_setup_command (notmuch_config_t *config,
chomp_newline (response);   \
 } while (0)
 
-notmuch_opt_desc_t options[] = {
-   { NOTMUCH_OPT_INHERIT, (void *) notmuch_shared_options, NULL, 0, 0 },
-   { 0, 0, 0, 0, 0 }
-};
-
-opt_index = parse_arguments (argc, argv, options, 1);
-if (opt_index  0)
+if (notmuch_minimal_options (setup, argc, argv)  0)
return EXIT_FAILURE;
 
-/* We can't use argv here as it is sometimes NULL */
-notmuch_process_shared_options (setup);
-
 if (notmuch_config_is_new (config))
welcome_message_pre_setup ();
 
diff --git a/notmuch.c b/notmuch.c
index c7f8c8f..2198b73 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -59,18 +59,40 @@ const notmuch_opt_desc_t notmuch_shared_options [] = {
  * notmuch_process_shared_options (subcommand_name);
  */
 void
-notmuch_process_shared_options (const char *help_name) {
+notmuch_process_shared_options (const char *subcommand_name) {
 if (print_version) {
printf (notmuch  STRINGIFY(NOTMUCH_VERSION) \n);
exit (EXIT_SUCCESS);
 }
 
 if (print_help) {
-   int ret = _help_for (help_name);
+   int ret = _help_for (subcommand_name);
exit (ret);
 }
 }
 
+/* This is suitable for subcommands that do not actually open the
+ * database.
+ */
+int notmuch_minimal_options (const char *subcommand_name,
+ int argc, char **argv)
+{
+int opt_index;
+
+notmuch_opt_desc_t options[] = {
+   { NOTMUCH_OPT_INHERIT, (void *) notmuch_shared_options, NULL, 0, 0 },
+   { 0, 0, 0, 0, 0 }
+};
+
+opt_index = parse_arguments (argc, argv, options, 1);
+
+if (opt_index  0)
+   return -1;
+
+/* We can't use argv here as it is sometimes NULL */
+notmuch_process_shared_options (subcommand_name);
+return opt_index;
+}
 
 static command_t commands[] = {
 { NULL, notmuch_command, TRUE,
@@ -250,7 +272,15 @@ _help_for (const char *topic_name)
 static int
 notmuch_help_command (unused (notmuch_config_t * config), int argc, char 
*argv[])
 {
-argc--; argv++; /* Ignore help */
+int opt_index;
+
+opt_index = notmuch_minimal_options (help, argc, argv);
+