[PATCH 0/3] ruby: improve db.query

[PATCH 0/3] ruby: improve db.query

[Felipe Contreras]

I find it a bit annoying to have to modify the query object to add options when
Notmuch::Database.query() can do that just fine.

This series also adds a mapping to the notmuch_exclude_t enum in order to be able to specify
NOTMUCH_EXCLUDE_ALL and others.

This patch series goes on top of my ruby test cleanups [1].

[1] id:20210501115923.483816-1-felipe.contreras@gmail.com

Felipe Contreras (3):
  ruby: use notmuch_exclude_t enum
  ruby: add keyword arguments to db.query
  test: ruby: simplify basic tests

 bindings/ruby/database.c | 47 +++++++++++++++++++++++++++++++++++++---
 bindings/ruby/defs.h     |  2 +-
 bindings/ruby/init.c     | 26 +++++++++++++++++++++-
 bindings/ruby/query.c    |  8 ++++---
 test/T395-ruby.sh        | 47 +++++++++++++++++++++++++++++++++-------
 5 files changed, 114 insertions(+), 16 deletions(-)

-- 
2.31.0

[PATCH 1/3] ruby: use notmuch_exclude_t enum

[Felipe Contreras]

It exists since 2013, let's allow it to be used in Ruby.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 bindings/ruby/init.c  | 24 ++++++++++++++++++++++++
 bindings/ruby/query.c |  8 +++++---
 test/T395-ruby.sh     | 17 +++++++++++++++++
 3 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/bindings/ruby/init.c b/bindings/ruby/init.c
index 819fd1e3..fd73ed8f 100644
--- a/bindings/ruby/init.c
+++ b/bindings/ruby/init.c
@@ -133,6 +133,30 @@ Init_notmuch (void)
      * Maximum allowed length of a tag
      */
     rb_define_const (mod, "TAG_MAX", INT2FIX (NOTMUCH_TAG_MAX));
+    /*
+     * Document-const: Notmuch::EXCLUDE_FLAG
+     *
+     * Only flag excluded results
+     */
+    rb_define_const (mod, "EXCLUDE_FLAG", INT2FIX (NOTMUCH_EXCLUDE_FLAG));
+    /*
+     * Document-const: Notmuch::EXCLUDE_TRUE
+     *
+     * Exclude messages from the results
+     */
+    rb_define_const (mod, "EXCLUDE_TRUE", INT2FIX (NOTMUCH_EXCLUDE_TRUE));
+    /*
+     * Document-const: Notmuch::EXCLUDE_FALSE
+     *
+     * Don't exclude anything
+     */
+    rb_define_const (mod, "EXCLUDE_FALSE", INT2FIX (NOTMUCH_EXCLUDE_FALSE));
+    /*
+     * Document-const: Notmuch::EXCLUDE_ALL
+     *
+     * Exclude all results
+     */
+    rb_define_const (mod, "EXCLUDE_ALL", INT2FIX (NOTMUCH_EXCLUDE_ALL));
 
     /*
      * Document-class: Notmuch::BaseError
diff --git a/bindings/ruby/query.c b/bindings/ruby/query.c
index 8b46d700..99148c36 100644
--- a/bindings/ruby/query.c
+++ b/bindings/ruby/query.c
@@ -107,19 +107,21 @@ notmuch_rb_query_add_tag_exclude (VALUE self, VALUE tagv)
 }
 
 /*
- * call-seq: QUERY.omit_excluded=(boolean) => nil
+ * call-seq: QUERY.omit_excluded=(fixnum) => nil
  *
  * Specify whether to omit excluded results or simply flag them.
- * By default, this is set to +true+.
+ * By default, this is set to +Notmuch::EXCLUDE_TRUE+.
  */
 VALUE
 notmuch_rb_query_set_omit_excluded (VALUE self, VALUE omitv)
 {
     notmuch_query_t *query;
+    notmuch_exclude_t value;
 
     Data_Get_Notmuch_Query (self, query);
 
-    notmuch_query_set_omit_excluded (query, RTEST (omitv));
+    value = FIXNUM_P (omitv) ? FIX2UINT (omitv) : RTEST(omitv);
+    notmuch_query_set_omit_excluded (query, value);
 
     return Qnil;
 }
diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
index 597330d3..d36d4aff 100755
--- a/test/T395-ruby.sh
+++ b/test/T395-ruby.sh
@@ -65,4 +65,21 @@ db.all_tags.each do |tag|
 end
 EOF
 
+notmuch config set search.exclude_tags deleted
+generate_message '[subject]="Good"'
+generate_message '[subject]="Bad"' "[in-reply-to]=\<$gen_msg_id\>"
+notmuch new > /dev/null
+notmuch tag +deleted id:$gen_msg_id
+
+test_begin_subtest "omit excluded all"
+notmuch search --output=threads --exclude=all tag:inbox > EXPECTED
+test_ruby <<"EOF"
+q = db.query('tag:inbox')
+q.add_tag_exclude('deleted')
+q.omit_excluded = Notmuch::EXCLUDE_ALL
+q.search_threads.each do |t|
+  puts 'thread:%s' % t.thread_id
+end
+EOF
+
 test_done
-- 
2.31.0

Re: [PATCH 1/3] ruby: use notmuch_exclude_t enum

[David Bremner]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> It exists since 2013, let's allow it to be used in Ruby.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

applied to master

d

[PATCH 2/3] ruby: add keyword arguments to db.query

[Felipe Contreras]

That way we don't need pass them to the query object ourselves.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 bindings/ruby/database.c | 47 +++++++++++++++++++++++++++++++++++++---
 bindings/ruby/defs.h     |  2 +-
 bindings/ruby/init.c     |  2 +-
 test/T395-ruby.sh        | 18 +++++++++++++++
 4 files changed, 64 insertions(+), 5 deletions(-)

diff --git a/bindings/ruby/database.c b/bindings/ruby/database.c
index 416eb709..313a4338 100644
--- a/bindings/ruby/database.c
+++ b/bindings/ruby/database.c
@@ -399,17 +399,24 @@ notmuch_rb_database_get_all_tags (VALUE self)
 }
 
 /*
- * call-seq: DB.query(query) => QUERY
+ * call-seq:
+ *   DB.query(query) => QUERY
+ *   DB.query(query, sort:, excluded_tags:, omit_excluded:) => QUERY
  *
- * Retrieve a query object for the query string 'query'
+ * Retrieve a query object for the query string 'query'. When using keyword
+ * arguments they are passwed to the query object.
  */
 VALUE
-notmuch_rb_database_query_create (VALUE self, VALUE qstrv)
+notmuch_rb_database_query_create (int argc, VALUE *argv, VALUE self)
 {
+    VALUE qstrv;
+    VALUE opts;
     const char *qstr;
     notmuch_query_t *query;
     notmuch_database_t *db;
 
+    rb_scan_args (argc, argv, "1:", &qstrv, &opts);
+
     Data_Get_Notmuch_Database (self, db);
 
     SafeStringValue (qstrv);
@@ -419,5 +426,39 @@ notmuch_rb_database_query_create (VALUE self, VALUE qstrv)
     if (!query)
         rb_raise (notmuch_rb_eMemoryError, "Out of memory");
 
+    if (!NIL_P (opts)) {
+	VALUE sort, exclude_tags, omit_excluded;
+	VALUE kwargs[3];
+	static ID keyword_ids[3];
+
+	if (!keyword_ids[0]) {
+	    keyword_ids[0] = rb_intern_const ("sort");
+	    keyword_ids[1] = rb_intern_const ("exclude_tags");
+	    keyword_ids[2] = rb_intern_const ("omit_excluded");
+	}
+
+	rb_get_kwargs (opts, keyword_ids, 0, 3, kwargs);
+
+	sort = kwargs[0];
+	exclude_tags = kwargs[1];
+	omit_excluded = kwargs[2];
+
+	if (sort != Qundef)
+	    notmuch_query_set_sort (query, FIX2UINT (sort));
+
+	if (exclude_tags != Qundef) {
+	    for (int i = 0; i < RARRAY_LEN (exclude_tags); i++) {
+		VALUE e = RARRAY_AREF (exclude_tags, i);
+		notmuch_query_add_tag_exclude (query, RSTRING_PTR (e));
+	    }
+	}
+
+	if (omit_excluded != Qundef) {
+	    notmuch_exclude_t omit;
+	    omit = FIXNUM_P (omit_excluded) ? FIX2UINT (omit_excluded) : RTEST(omit_excluded);
+	    notmuch_query_set_omit_excluded (query, omit);
+	}
+    }
+
     return Data_Wrap_Struct (notmuch_rb_cQuery, NULL, NULL, query);
 }
diff --git a/bindings/ruby/defs.h b/bindings/ruby/defs.h
index 48544ca2..1ceaadb5 100644
--- a/bindings/ruby/defs.h
+++ b/bindings/ruby/defs.h
@@ -181,7 +181,7 @@ VALUE
 notmuch_rb_database_get_all_tags (VALUE self);
 
 VALUE
-notmuch_rb_database_query_create (VALUE self, VALUE qstrv);
+notmuch_rb_database_query_create (int argc, VALUE *argv, VALUE self);
 
 /* directory.c */
 VALUE
diff --git a/bindings/ruby/init.c b/bindings/ruby/init.c
index fd73ed8f..f509edbd 100644
--- a/bindings/ruby/init.c
+++ b/bindings/ruby/init.c
@@ -254,7 +254,7 @@ Init_notmuch (void)
     rb_define_method (notmuch_rb_cDatabase, "find_message_by_filename",
 		      notmuch_rb_database_find_message_by_filename, 1); /* in database.c */
     rb_define_method (notmuch_rb_cDatabase, "all_tags", notmuch_rb_database_get_all_tags, 0); /* in database.c */
-    rb_define_method (notmuch_rb_cDatabase, "query", notmuch_rb_database_query_create, 1); /* in database.c */
+    rb_define_method (notmuch_rb_cDatabase, "query", notmuch_rb_database_query_create, -1); /* in database.c */
 
     /*
      * Document-class: Notmuch::Directory
diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
index d36d4aff..e828efed 100755
--- a/test/T395-ruby.sh
+++ b/test/T395-ruby.sh
@@ -82,4 +82,22 @@ q.search_threads.each do |t|
 end
 EOF
 
+test_begin_subtest "check sort argument"
+notmuch search --sort=oldest-first --output=threads tag:inbox > EXPECTED
+test_ruby <<"EOF"
+q = db.query('tag:inbox', sort: Notmuch::SORT_OLDEST_FIRST)
+q.search_threads.each do |t|
+  puts 'thread:%s' % t.thread_id
+end
+EOF
+
+test_begin_subtest "check exclude_tags argument"
+notmuch search --output=threads --exclude=all tag:inbox > EXPECTED
+test_ruby <<"EOF"
+q = db.query('tag:inbox', exclude_tags: %w[deleted], omit_excluded: Notmuch::EXCLUDE_ALL)
+q.search_threads.each do |t|
+  puts 'thread:%s' % t.thread_id
+end
+EOF
+
 test_done
-- 
2.31.0

Re: [PATCH 2/3] ruby: add keyword arguments to db.query

[David Bremner]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> That way we don't need pass them to the query object ourselves.

LGTM, but when I tried to resolve the conflicts I messed something up
and the ruby sort test(s) started failing.  Rebase please?

d

Re: [PATCH 2/3] ruby: add keyword arguments to db.query

[Felipe Contreras]

On Sun, May 23, 2021 at 7:29 AM David Bremner <david@tethera.net> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > That way we don't need pass them to the query object ourselves.
>
> LGTM, but when I tried to resolve the conflicts I messed something up
> and the ruby sort test(s) started failing.  Rebase please?

Not sure what happened there, my rebase happened without conflicts.

-- 
Felipe Contreras

[PATCH 3/3] test: ruby: simplify basic tests

[Felipe Contreras]

We don't need to check for the order here, that is done in another test.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 test/T395-ruby.sh | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
index e828efed..9298bc9e 100755
--- a/test/T395-ruby.sh
+++ b/test/T395-ruby.sh
@@ -20,21 +20,17 @@ test_ruby() {
 }
 
 test_begin_subtest "compare thread ids"
-notmuch search --sort=oldest-first --output=threads tag:inbox > EXPECTED
+notmuch search --output=threads tag:inbox > EXPECTED
 test_ruby <<"EOF"
-q = db.query('tag:inbox')
-q.sort = Notmuch::SORT_OLDEST_FIRST
-q.search_threads.each do |t|
+db.query('tag:inbox').search_threads.each do |t|
   puts 'thread:%s' % t.thread_id
 end
 EOF
 
 test_begin_subtest "compare message ids"
-notmuch search --sort=oldest-first --output=messages tag:inbox > EXPECTED
+notmuch search --output=messages tag:inbox > EXPECTED
 test_ruby <<"EOF"
-q = db.query('tag:inbox')
-q.sort = Notmuch::SORT_OLDEST_FIRST
-q.search_messages.each do |m|
+db.query('tag:inbox').search_messages.each do |m|
   puts 'id:%s' % m.message_id
 end
 EOF
-- 
2.31.0

Re: [PATCH 3/3] test: ruby: simplify basic tests

[David Bremner]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> We don't need to check for the order here, that is done in another test.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  test/T395-ruby.sh | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
> index e828efed..9298bc9e 100755
> --- a/test/T395-ruby.sh
> +++ b/test/T395-ruby.sh
> @@ -20,21 +20,17 @@ test_ruby() {
>  }
>  
>  test_begin_subtest "compare thread ids"
> -notmuch search --sort=oldest-first --output=threads tag:inbox > EXPECTED
> +notmuch search --output=threads tag:inbox > EXPECTED
>  test_ruby <<"EOF"
> -q = db.query('tag:inbox')
> -q.sort = Notmuch::SORT_OLDEST_FIRST
> -q.search_threads.each do |t|
> +db.query('tag:inbox').search_threads.each do |t|
>    puts 'thread:%s' % t.thread_id
>  end
>  EOF
>  

Is this assuming that the sort order in the CLI is the same as in the
library / bindings? that seems a bit fragile if so.

d

Re: [PATCH 3/3] test: ruby: simplify basic tests

[Felipe Contreras]

On Sun, May 23, 2021 at 7:32 AM David Bremner <david@tethera.net> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > We don't need to check for the order here, that is done in another test.
> >
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  test/T395-ruby.sh | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
> > index e828efed..9298bc9e 100755
> > --- a/test/T395-ruby.sh
> > +++ b/test/T395-ruby.sh
> > @@ -20,21 +20,17 @@ test_ruby() {
> >  }
> >
> >  test_begin_subtest "compare thread ids"
> > -notmuch search --sort=oldest-first --output=threads tag:inbox > EXPECTED
> > +notmuch search --output=threads tag:inbox > EXPECTED
> >  test_ruby <<"EOF"
> > -q = db.query('tag:inbox')
> > -q.sort = Notmuch::SORT_OLDEST_FIRST
> > -q.search_threads.each do |t|
> > +db.query('tag:inbox').search_threads.each do |t|
> >    puts 'thread:%s' % t.thread_id
> >  end
> >  EOF
> >
>
> Is this assuming that the sort order in the CLI is the same as in the
> library / bindings? that seems a bit fragile if so.

Both the CLI and the bindings are using the same libnotmuch library.
If neither of them specify a sort order, the default sort order of
libnotmuch would be used (I presume). Exactly the same order I would
get if I write a C program that uses libnotmuch and doesn't specify
any order.

Why would the CLI specify an order the user didn't specify to libnotmuch?

-- 
Felipe Contreras

Re: [PATCH 3/3] test: ruby: simplify basic tests

[David Bremner]

Felipe Contreras <felipe.contreras@gmail.com> writes:

>>
>> Is this assuming that the sort order in the CLI is the same as in the
>> library / bindings? that seems a bit fragile if so.
>
> Both the CLI and the bindings are using the same libnotmuch library.
> If neither of them specify a sort order, the default sort order of
> libnotmuch would be used (I presume). Exactly the same order I would
> get if I write a C program that uses libnotmuch and doesn't specify
> any order.
>
> Why would the CLI specify an order the user didn't specify to libnotmuch?

I guess the point is that the CLI is not required to track the library
precisely, so even if it is a bit theoretical, this change does
introduce some fragility / technical debt into the test suite.

For better or for worse, the API does not document a default sort order,
so assuming any particular sort order is probably a mistake. I can
imagine a future database backend where the "UNSORTED" order is actually
non-deterministic. If we were to document a default sort order, UNSORTED
might make the most sense as a default, as it's the highest performance
(more or less by definition).

Re: [PATCH 3/3] test: ruby: simplify basic tests

[Felipe Contreras]

On Mon, Jun 7, 2021 at 6:53 PM David Bremner <david@tethera.net> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> >>
> >> Is this assuming that the sort order in the CLI is the same as in the
> >> library / bindings? that seems a bit fragile if so.
> >
> > Both the CLI and the bindings are using the same libnotmuch library.
> > If neither of them specify a sort order, the default sort order of
> > libnotmuch would be used (I presume). Exactly the same order I would
> > get if I write a C program that uses libnotmuch and doesn't specify
> > any order.
> >
> > Why would the CLI specify an order the user didn't specify to libnotmuch?
>
> I guess the point is that the CLI is not required to track the library
> precisely, so even if it is a bit theoretical, this change does
> introduce some fragility / technical debt into the test suite.
>
> For better or for worse, the API does not document a default sort order,
> so assuming any particular sort order is probably a mistake. I can
> imagine a future database backend where the "UNSORTED" order is actually
> non-deterministic. If we were to document a default sort order, UNSORTED
> might make the most sense as a default, as it's the highest performance
> (more or less by definition).

We could have test_ruby_unordered and sort both the expected and
actual files to ensure the tests don't break if in the future the
default order changes. But personally I don't see much point in
complicating the tests preemptively. I would rather worry when the
time comes, which might be never.

-- 
Felipe Contreras