[PATCH 0/3] ruby: get rid of FileNames object

[PATCH 0/3] ruby: get rid of FileNames object

[Felipe Contreras]

We don't need a FileNames enumerable object only for a small number of strings,
we can just get them directly.

This iterator is meant to be transient and works only once, so we better just
iterate it once.

This is the same approach I took with the Tags object, I was waiting for
feedback on that approach but since there isn't any and there's no reason this
shouldn't work, here's the same for Filenames.

Felipe Contreras (3):
  ruby: add filenames helper
  ruby: filenames: return string array directly
  ruby: remove FileNames object

 bindings/ruby/defs.h      |  9 +------
 bindings/ruby/directory.c |  4 +--
 bindings/ruby/filenames.c | 51 +++------------------------------------
 bindings/ruby/init.c      | 14 -----------
 bindings/ruby/message.c   |  2 +-
 5 files changed, 8 insertions(+), 72 deletions(-)

-- 
2.39.2.13.g1fb56cf030

[PATCH 1/3] ruby: add filenames helper

[Felipe Contreras]

Right now it doesn't do much, but it will help for further
reorganization.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 bindings/ruby/defs.h      | 3 +++
 bindings/ruby/directory.c | 4 ++--
 bindings/ruby/filenames.c | 6 ++++++
 bindings/ruby/message.c   | 2 +-
 4 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/bindings/ruby/defs.h b/bindings/ruby/defs.h
index e2541e8f..216380d4 100644
--- a/bindings/ruby/defs.h
+++ b/bindings/ruby/defs.h
@@ -225,6 +225,9 @@ VALUE
 notmuch_rb_directory_get_child_directories (VALUE self);
 
 /* filenames.c */
+VALUE
+notmuch_rb_filenames_get (notmuch_filenames_t *fnames);
+
 VALUE
 notmuch_rb_filenames_destroy (VALUE self);
 
diff --git a/bindings/ruby/directory.c b/bindings/ruby/directory.c
index 910f0a99..f267d82f 100644
--- a/bindings/ruby/directory.c
+++ b/bindings/ruby/directory.c
@@ -87,7 +87,7 @@ notmuch_rb_directory_get_child_files (VALUE self)
 
     fnames = notmuch_directory_get_child_files (dir);
 
-    return Data_Wrap_Notmuch_Object (notmuch_rb_cFileNames, &notmuch_rb_filenames_type, fnames);
+    return notmuch_rb_filenames_get (fnames);
 }
 
 /*
@@ -106,5 +106,5 @@ notmuch_rb_directory_get_child_directories (VALUE self)
 
     fnames = notmuch_directory_get_child_directories (dir);
 
-    return Data_Wrap_Notmuch_Object (notmuch_rb_cFileNames, &notmuch_rb_filenames_type, fnames);
+    return notmuch_rb_filenames_get (fnames);
 }
diff --git a/bindings/ruby/filenames.c b/bindings/ruby/filenames.c
index 0dec1952..17873393 100644
--- a/bindings/ruby/filenames.c
+++ b/bindings/ruby/filenames.c
@@ -20,6 +20,12 @@
 
 #include "defs.h"
 
+VALUE
+notmuch_rb_filenames_get (notmuch_filenames_t *fnames)
+{
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cFileNames, &notmuch_rb_filenames_type, fnames);
+}
+
 /*
  * call-seq: FILENAMES.destroy! => nil
  *
diff --git a/bindings/ruby/message.c b/bindings/ruby/message.c
index f45c95cc..0d83567c 100644
--- a/bindings/ruby/message.c
+++ b/bindings/ruby/message.c
@@ -120,7 +120,7 @@ notmuch_rb_message_get_filenames (VALUE self)
 
     fnames = notmuch_message_get_filenames (message);
 
-    return Data_Wrap_Notmuch_Object (notmuch_rb_cFileNames, &notmuch_rb_filenames_type, fnames);
+    return notmuch_rb_filenames_get (fnames);
 }
 
 /*
-- 
2.39.2.13.g1fb56cf030

[PATCH 2/3] ruby: filenames: return string array directly

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 bindings/ruby/filenames.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/bindings/ruby/filenames.c b/bindings/ruby/filenames.c
index 17873393..17ec4406 100644
--- a/bindings/ruby/filenames.c
+++ b/bindings/ruby/filenames.c
@@ -23,7 +23,10 @@
 VALUE
 notmuch_rb_filenames_get (notmuch_filenames_t *fnames)
 {
-    return Data_Wrap_Notmuch_Object (notmuch_rb_cFileNames, &notmuch_rb_filenames_type, fnames);
+    VALUE rb_array = rb_ary_new();
+    for (; notmuch_filenames_valid (fnames); notmuch_filenames_move_to_next (fnames))
+	rb_ary_push (rb_array, rb_str_new2 (notmuch_filenames_get (fnames)));
+    return rb_array;
 }
 
 /*
-- 
2.39.2.13.g1fb56cf030

[PATCH 3/3] ruby: remove FileNames object

[Felipe Contreras]

Not used anymore now that we return an array of strings directly.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 bindings/ruby/defs.h      | 10 --------
 bindings/ruby/filenames.c | 52 ---------------------------------------
 bindings/ruby/init.c      | 14 -----------
 3 files changed, 76 deletions(-)

diff --git a/bindings/ruby/defs.h b/bindings/ruby/defs.h
index 216380d4..2fde8652 100644
--- a/bindings/ruby/defs.h
+++ b/bindings/ruby/defs.h
@@ -59,7 +59,6 @@ extern ID ID_db_mode;
 extern const rb_data_type_t notmuch_rb_object_type;
 extern const rb_data_type_t notmuch_rb_database_type;
 extern const rb_data_type_t notmuch_rb_directory_type;
-extern const rb_data_type_t notmuch_rb_filenames_type;
 extern const rb_data_type_t notmuch_rb_query_type;
 extern const rb_data_type_t notmuch_rb_threads_type;
 extern const rb_data_type_t notmuch_rb_thread_type;
@@ -92,9 +91,6 @@ extern const rb_data_type_t notmuch_rb_tags_type;
 #define Data_Get_Notmuch_Directory(obj, ptr) \
     Data_Get_Notmuch_Object ((obj), &notmuch_rb_directory_type, (ptr))
 
-#define Data_Get_Notmuch_FileNames(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), &notmuch_rb_filenames_type, (ptr))
-
 #define Data_Get_Notmuch_Query(obj, ptr) \
     Data_Get_Notmuch_Object ((obj), &notmuch_rb_query_type, (ptr))
 
@@ -228,12 +224,6 @@ notmuch_rb_directory_get_child_directories (VALUE self);
 VALUE
 notmuch_rb_filenames_get (notmuch_filenames_t *fnames);
 
-VALUE
-notmuch_rb_filenames_destroy (VALUE self);
-
-VALUE
-notmuch_rb_filenames_each (VALUE self);
-
 /* query.c */
 VALUE
 notmuch_rb_query_destroy (VALUE self);
diff --git a/bindings/ruby/filenames.c b/bindings/ruby/filenames.c
index 17ec4406..7ff33dca 100644
--- a/bindings/ruby/filenames.c
+++ b/bindings/ruby/filenames.c
@@ -1,23 +1,3 @@
-/* The Ruby interface to the notmuch mail library
- *
- * Copyright © 2010 Ali Polatel
- *
- * This program is free software: you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation, either version 3 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see https://www.gnu.org/licenses/ .
- *
- * Author: Ali Polatel <alip@exherbo.org>
- */
-
 #include "defs.h"
 
 VALUE
@@ -28,35 +8,3 @@ notmuch_rb_filenames_get (notmuch_filenames_t *fnames)
 	rb_ary_push (rb_array, rb_str_new2 (notmuch_filenames_get (fnames)));
     return rb_array;
 }
-
-/*
- * call-seq: FILENAMES.destroy! => nil
- *
- * Destroys the filenames, freeing all resources allocated for it.
- */
-VALUE
-notmuch_rb_filenames_destroy (VALUE self)
-{
-    notmuch_rb_object_destroy (self, &notmuch_rb_filenames_type);
-
-    return Qnil;
-}
-
-/*
- * call-seq: FILENAMES.each {|item| block } => FILENAMES
- *
- * Calls +block+ once for each element in +self+, passing that element as a
- * parameter.
- */
-VALUE
-notmuch_rb_filenames_each (VALUE self)
-{
-    notmuch_filenames_t *fnames;
-
-    Data_Get_Notmuch_FileNames (self, fnames);
-
-    for (; notmuch_filenames_valid (fnames); notmuch_filenames_move_to_next (fnames))
-	rb_yield (rb_str_new2 (notmuch_filenames_get (fnames)));
-
-    return self;
-}
diff --git a/bindings/ruby/init.c b/bindings/ruby/init.c
index cd9f04cd..816b8cf5 100644
--- a/bindings/ruby/init.c
+++ b/bindings/ruby/init.c
@@ -22,7 +22,6 @@
 
 VALUE notmuch_rb_cDatabase;
 VALUE notmuch_rb_cDirectory;
-VALUE notmuch_rb_cFileNames;
 VALUE notmuch_rb_cQuery;
 VALUE notmuch_rb_cThreads;
 VALUE notmuch_rb_cThread;
@@ -65,7 +64,6 @@ const rb_data_type_t notmuch_rb_object_type = {
 
 define_type (database);
 define_type (directory);
-define_type (filenames);
 define_type (query);
 define_type (threads);
 define_type (thread);
@@ -86,7 +84,6 @@ define_type (tags);
  * the user:
  *
  * - Notmuch::Database
- * - Notmuch::FileNames
  * - Notmuch::Query
  * - Notmuch::Threads
  * - Notmuch::Messages
@@ -297,17 +294,6 @@ Init_notmuch (void)
     rb_define_method (notmuch_rb_cDirectory, "child_files", notmuch_rb_directory_get_child_files, 0); /* in directory.c */
     rb_define_method (notmuch_rb_cDirectory, "child_directories", notmuch_rb_directory_get_child_directories, 0); /* in directory.c */
 
-    /*
-     * Document-class: Notmuch::FileNames
-     *
-     * Notmuch file names
-     */
-    notmuch_rb_cFileNames = rb_define_class_under (mod, "FileNames", rb_cObject);
-    rb_undef_method (notmuch_rb_cFileNames, "initialize");
-    rb_define_method (notmuch_rb_cFileNames, "destroy!", notmuch_rb_filenames_destroy, 0); /* in filenames.c */
-    rb_define_method (notmuch_rb_cFileNames, "each", notmuch_rb_filenames_each, 0); /* in filenames.c */
-    rb_include_module (notmuch_rb_cFileNames, rb_mEnumerable);
-
     /*
      * Document-class: Notmuch::Query
      *
-- 
2.39.2.13.g1fb56cf030
\r

Re: [PATCH 0/3] ruby: get rid of FileNames object

[David Bremner]

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

> We don't need a FileNames enumerable object only for a small number of strings,
> we can just get them directly.
>
> This iterator is meant to be transient and works only once, so we better just
> iterate it once.
>
> This is the same approach I took with the Tags object, I was waiting for
> feedback on that approach but since there isn't any and there's no reason this
> shouldn't work, here's the same for Filenames.

Hi Felipe;

I still haven't had a chance to look at the proposed changes, but I did
wonder what your plan was as far as migration. Usually with the library
itself we try to provide fairly smooth upgrades.

d

Re: [PATCH 0/3] ruby: get rid of FileNames object

[Felipe Contreras]

On Mon, Mar 27, 2023 at 6:13 PM David Bremner <david@tethera.net> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > We don't need a FileNames enumerable object only for a small number of strings,
> > we can just get them directly.
> >
> > This iterator is meant to be transient and works only once, so we better just
> > iterate it once.
> >
> > This is the same approach I took with the Tags object, I was waiting for
> > feedback on that approach but since there isn't any and there's no reason this
> > shouldn't work, here's the same for Filenames.
>
> Hi Felipe;
>
> I still haven't had a chance to look at the proposed changes, but I did
> wonder what your plan was as far as migration. Usually with the library
> itself we try to provide fairly smooth upgrades.

There's no migration, there's no functional changes from clients' point of view.

Previously the code returned a Notmuch::Tags object, but this class
included the Enumerable [1] module, making it an Enumerable. So
everything you could do with an Enumerable you could do with
Notmuch::Tags, for example: tags.count().

But we didn't implement all these methods, we only had to implement
`each()`. If `each()` works, then everything else works.

For example this class implements an Enumerable:

  class Foo
    include Enumerable
    def each
      yield 'inbox'
      yield 'unread'
    end
  end

And then all these work:

  foo = Foo.new
  foo.each { |e| puts e }
  puts 'first: %s' % foo.first
  puts 'count: %s' % foo.count

We can replace the Foo object any other kind of Enumerable, and the
code works just the same:

  foo = %w[inbox unread]
  foo.each { |e| puts e }
  puts 'first: %s' % foo.first
  puts 'count: %s' % foo.count

So replacing Notmuch::Tags with an array of strings doesn't change
anything because both are Enumerable, and both yield Strings.

Cheers.

[1] https://rubydoc.info/stdlib/core/Enumerable

-- 
Felipe Contreras\r

Re: [PATCH 0/3] ruby: get rid of FileNames object

[Felipe Contreras]

On Tue, Mar 28, 2023 at 12:47 AM Felipe Contreras
<felipe.contreras@gmail.com> wrote:

> We can replace the Foo object any other kind of Enumerable, and the
> code works just the same:
>
>   foo = %w[inbox unread]

I realized this might be too idiomatic of Ruby, it's the same thing as:

foo = [ 'inbox', 'unread' ]

>   foo.each { |e| puts e }
>   puts 'first: %s' % foo.first
>   puts 'count: %s' % foo.count

-- 
Felipe Contreras\r

Re: [PATCH 3/3] ruby: remove FileNames object

[David Bremner]

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

> Not used anymore now that we return an array of strings directly.

series applied to master, with one whitespace tweak in patch 2/3