[PATCH 0/9] test: ruby: several cleanups and simplifications

[PATCH 0/9] test: ruby: several cleanups and simplifications

[Felipe Contreras]

I found a lot of areas of improvement in the Ruby tests, so I decided to clean them up.

With these changes the tests are now much simpler and follow more closely the typical Ruby idioms.

Felipe Contreras (9):
  test: move test_ruby() inside the only client
  test: ruby: refactor test_ruby()
  test: ruby: simplify MAIL_DIR check
  test: ruby: simplify MAIL_DIR initialization
  test: ruby: simplify test_ruby()
  test: ruby: use much more standard puts
  test: ruby: use much more standard Ruby idioms
  test: ruby: don't use instance variables
  test: ruby: simplify output comparison

 test/T395-ruby.sh | 94 +++++++++++++++--------------------------------
 test/test-lib.sh  |  4 --
 2 files changed, 30 insertions(+), 68 deletions(-)

-- 
2.31.0

[PATCH 1/9] test: move test_ruby() inside the only client

[Felipe Contreras]

Not much point in polluting the main library, and also will be useful to
modify it in tandem with the tests.

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

diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
index a0b76eb8..fec1f5ef 100755
--- a/test/T395-ruby.sh
+++ b/test/T395-ruby.sh
@@ -8,6 +8,10 @@ fi
 
 add_email_corpus
 
+test_ruby() {
+    MAIL_DIR=$MAIL_DIR $NOTMUCH_RUBY -I "$NOTMUCH_BUILDDIR/bindings/ruby"> OUTPUT
+}
+
 test_begin_subtest "compare thread ids"
 test_ruby <<"EOF"
 require 'notmuch'
diff --git a/test/test-lib.sh b/test/test-lib.sh
index 4c9f2a21..ec0ba7f7 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -1109,10 +1109,6 @@ test_python() {
 	$NOTMUCH_PYTHON -B - > OUTPUT
 }
 
-test_ruby() {
-    MAIL_DIR=$MAIL_DIR $NOTMUCH_RUBY -I "$NOTMUCH_BUILDDIR/bindings/ruby"> OUTPUT
-}
-
 test_C () {
     local exec_file test_file
     exec_file="test${test_count}"
-- 
2.31.0

[PATCH 2/9] test: ruby: refactor test_ruby()

[Felipe Contreras]

There's no point in repeating the same initialization in all the tests.

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

diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
index fec1f5ef..1d27e191 100755
--- a/test/T395-ruby.sh
+++ b/test/T395-ruby.sh
@@ -9,17 +9,21 @@ fi
 add_email_corpus
 
 test_ruby() {
-    MAIL_DIR=$MAIL_DIR $NOTMUCH_RUBY -I "$NOTMUCH_BUILDDIR/bindings/ruby"> OUTPUT
+    (
+	cat <<-\EOF
+	require 'notmuch'
+	$maildir = ENV['MAIL_DIR']
+	if not $maildir then
+	  abort('environment variable MAIL_DIR must be set')
+	end
+	@db = Notmuch::Database.new($maildir)
+	EOF
+	cat
+    ) | MAIL_DIR=$MAIL_DIR $NOTMUCH_RUBY -I "$NOTMUCH_BUILDDIR/bindings/ruby"> OUTPUT
 }
 
 test_begin_subtest "compare thread ids"
 test_ruby <<"EOF"
-require 'notmuch'
-$maildir = ENV['MAIL_DIR']
-if not $maildir then
-  abort('environment variable MAIL_DIR must be set')
-end
-@db = Notmuch::Database.new($maildir)
 @q = @db.query('tag:inbox')
 @q.sort = Notmuch::SORT_OLDEST_FIRST
 for t in @q.search_threads do
@@ -31,12 +35,6 @@ test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "compare message ids"
 test_ruby <<"EOF"
-require 'notmuch'
-$maildir = ENV['MAIL_DIR']
-if not $maildir then
-  abort('environment variable MAIL_DIR must be set')
-end
-@db = Notmuch::Database.new($maildir)
 @q = @db.query('tag:inbox')
 @q.sort = Notmuch::SORT_OLDEST_FIRST
 for m in @q.search_messages do
@@ -48,12 +46,6 @@ test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "get non-existent file"
 test_ruby <<"EOF"
-require 'notmuch'
-$maildir = ENV['MAIL_DIR']
-if not $maildir then
-  abort('environment variable MAIL_DIR must be set')
-end
-@db = Notmuch::Database.new($maildir)
 result = @db.find_message_by_filename('i-dont-exist')
 print (result == nil)
 EOF
@@ -61,12 +53,6 @@ test_expect_equal "$(cat OUTPUT)" "true"
 
 test_begin_subtest "count messages"
 test_ruby <<"EOF"
-require 'notmuch'
-$maildir = ENV['MAIL_DIR']
-if not $maildir then
-  abort('environment variable MAIL_DIR must be set')
-end
-@db = Notmuch::Database.new($maildir)
 @q = @db.query('tag:inbox')
 print @q.count_messages(),"\n"
 EOF
@@ -75,12 +61,6 @@ test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "count threads"
 test_ruby <<"EOF"
-require 'notmuch'
-$maildir = ENV['MAIL_DIR']
-if not $maildir then
-  abort('environment variable MAIL_DIR must be set')
-end
-@db = Notmuch::Database.new($maildir)
 @q = @db.query('tag:inbox')
 print @q.count_threads(),"\n"
 EOF
@@ -89,12 +69,6 @@ test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "get all tags"
 test_ruby <<"EOF"
-require 'notmuch'
-$maildir = ENV['MAIL_DIR']
-if not $maildir then
-  abort('environment variable MAIL_DIR must be set')
-end
-@db = Notmuch::Database.new($maildir)
 @t = @db.all_tags()
 for tag in @t do
    print tag,"\n"
-- 
2.31.0

[PATCH 3/9] test: ruby: simplify MAIL_DIR check

[Felipe Contreras]

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

diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
index 1d27e191..94fab106 100755
--- a/test/T395-ruby.sh
+++ b/test/T395-ruby.sh
@@ -12,10 +12,7 @@ test_ruby() {
     (
 	cat <<-\EOF
 	require 'notmuch'
-	$maildir = ENV['MAIL_DIR']
-	if not $maildir then
-	  abort('environment variable MAIL_DIR must be set')
-	end
+	$maildir = ENV['MAIL_DIR'] || abort('MAIL_DIR not set')
 	@db = Notmuch::Database.new($maildir)
 	EOF
 	cat
-- 
2.31.0

[PATCH 4/9] test: ruby: simplify MAIL_DIR initialization

[Felipe Contreras]

There's no need to complicate the script passing the MAIL_DIR
environment variable.

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

diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
index 94fab106..67d6e205 100755
--- a/test/T395-ruby.sh
+++ b/test/T395-ruby.sh
@@ -10,13 +10,12 @@ add_email_corpus
 
 test_ruby() {
     (
-	cat <<-\EOF
+	cat <<-EOF
 	require 'notmuch'
-	$maildir = ENV['MAIL_DIR'] || abort('MAIL_DIR not set')
-	@db = Notmuch::Database.new($maildir)
+	@db = Notmuch::Database.new('$MAIL_DIR')
 	EOF
 	cat
-    ) | MAIL_DIR=$MAIL_DIR $NOTMUCH_RUBY -I "$NOTMUCH_BUILDDIR/bindings/ruby"> OUTPUT
+    ) | $NOTMUCH_RUBY -I "$NOTMUCH_BUILDDIR/bindings/ruby"> OUTPUT
 }
 
 test_begin_subtest "compare thread ids"
-- 
2.31.0

[PATCH 5/9] test: ruby: simplify test_ruby()

[Felipe Contreras]

We always do test_expect_equal_file, so do it in test_ruby() directly.

The only subtest where we don't (get non-existent file) can be easily
modified.

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

diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
index 67d6e205..55bf4c2b 100755
--- a/test/T395-ruby.sh
+++ b/test/T395-ruby.sh
@@ -16,9 +16,11 @@ test_ruby() {
 	EOF
 	cat
     ) | $NOTMUCH_RUBY -I "$NOTMUCH_BUILDDIR/bindings/ruby"> OUTPUT
+    test_expect_equal_file EXPECTED OUTPUT
 }
 
 test_begin_subtest "compare thread ids"
+notmuch search --sort=oldest-first --output=threads tag:inbox | sed s/^thread:// > EXPECTED
 test_ruby <<"EOF"
 @q = @db.query('tag:inbox')
 @q.sort = Notmuch::SORT_OLDEST_FIRST
@@ -26,10 +28,9 @@ for t in @q.search_threads do
   print t.thread_id, "\n"
 end
 EOF
-notmuch search --sort=oldest-first --output=threads tag:inbox | sed s/^thread:// > EXPECTED
-test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "compare message ids"
+notmuch search --sort=oldest-first --output=messages tag:inbox | sed s/^id:// > EXPECTED
 test_ruby <<"EOF"
 @q = @db.query('tag:inbox')
 @q.sort = Notmuch::SORT_OLDEST_FIRST
@@ -37,40 +38,35 @@ for m in @q.search_messages do
   print m.message_id, "\n"
 end
 EOF
-notmuch search --sort=oldest-first --output=messages tag:inbox | sed s/^id:// > EXPECTED
-test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "get non-existent file"
+echo -n true > EXPECTED
 test_ruby <<"EOF"
 result = @db.find_message_by_filename('i-dont-exist')
 print (result == nil)
 EOF
-test_expect_equal "$(cat OUTPUT)" "true"
 
 test_begin_subtest "count messages"
+notmuch count --output=messages tag:inbox > EXPECTED
 test_ruby <<"EOF"
 @q = @db.query('tag:inbox')
 print @q.count_messages(),"\n"
 EOF
-notmuch count --output=messages tag:inbox > EXPECTED
-test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "count threads"
+notmuch count --output=threads tag:inbox > EXPECTED
 test_ruby <<"EOF"
 @q = @db.query('tag:inbox')
 print @q.count_threads(),"\n"
 EOF
-notmuch count --output=threads tag:inbox > EXPECTED
-test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "get all tags"
+notmuch search --output=tags '*' > EXPECTED
 test_ruby <<"EOF"
 @t = @db.all_tags()
 for tag in @t do
    print tag,"\n"
 end
 EOF
-notmuch search --output=tags '*' > EXPECTED
-test_expect_equal_file EXPECTED OUTPUT
 
 test_done
-- 
2.31.0

[PATCH 6/9] test: ruby: use much more standard puts

[Felipe Contreras]

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

diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
index 55bf4c2b..f871ddd9 100755
--- a/test/T395-ruby.sh
+++ b/test/T395-ruby.sh
@@ -25,7 +25,7 @@ test_ruby <<"EOF"
 @q = @db.query('tag:inbox')
 @q.sort = Notmuch::SORT_OLDEST_FIRST
 for t in @q.search_threads do
-  print t.thread_id, "\n"
+  puts t.thread_id
 end
 EOF
 
@@ -35,29 +35,29 @@ test_ruby <<"EOF"
 @q = @db.query('tag:inbox')
 @q.sort = Notmuch::SORT_OLDEST_FIRST
 for m in @q.search_messages do
-  print m.message_id, "\n"
+  puts m.message_id
 end
 EOF
 
 test_begin_subtest "get non-existent file"
-echo -n true > EXPECTED
+echo true > EXPECTED
 test_ruby <<"EOF"
 result = @db.find_message_by_filename('i-dont-exist')
-print (result == nil)
+puts (result == nil)
 EOF
 
 test_begin_subtest "count messages"
 notmuch count --output=messages tag:inbox > EXPECTED
 test_ruby <<"EOF"
 @q = @db.query('tag:inbox')
-print @q.count_messages(),"\n"
+puts @q.count_messages()
 EOF
 
 test_begin_subtest "count threads"
 notmuch count --output=threads tag:inbox > EXPECTED
 test_ruby <<"EOF"
 @q = @db.query('tag:inbox')
-print @q.count_threads(),"\n"
+puts @q.count_threads()
 EOF
 
 test_begin_subtest "get all tags"
@@ -65,7 +65,7 @@ notmuch search --output=tags '*' > EXPECTED
 test_ruby <<"EOF"
 @t = @db.all_tags()
 for tag in @t do
-   print tag,"\n"
+   puts tag
 end
 EOF
 
-- 
2.31.0

[PATCH 7/9] test: ruby: use much more standard Ruby idioms

[Felipe Contreras]

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

diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
index f871ddd9..f5a8d245 100755
--- a/test/T395-ruby.sh
+++ b/test/T395-ruby.sh
@@ -24,7 +24,7 @@ notmuch search --sort=oldest-first --output=threads tag:inbox | sed s/^thread://
 test_ruby <<"EOF"
 @q = @db.query('tag:inbox')
 @q.sort = Notmuch::SORT_OLDEST_FIRST
-for t in @q.search_threads do
+@q.search_threads.each do |t|
   puts t.thread_id
 end
 EOF
@@ -34,38 +34,34 @@ notmuch search --sort=oldest-first --output=messages tag:inbox | sed s/^id:// >
 test_ruby <<"EOF"
 @q = @db.query('tag:inbox')
 @q.sort = Notmuch::SORT_OLDEST_FIRST
-for m in @q.search_messages do
+@q.search_messages.each do |m|
   puts m.message_id
 end
 EOF
 
 test_begin_subtest "get non-existent file"
-echo true > EXPECTED
+echo nil > EXPECTED
 test_ruby <<"EOF"
-result = @db.find_message_by_filename('i-dont-exist')
-puts (result == nil)
+p @db.find_message_by_filename('i-dont-exist')
 EOF
 
 test_begin_subtest "count messages"
 notmuch count --output=messages tag:inbox > EXPECTED
 test_ruby <<"EOF"
-@q = @db.query('tag:inbox')
-puts @q.count_messages()
+puts @db.query('tag:inbox').count_messages()
 EOF
 
 test_begin_subtest "count threads"
 notmuch count --output=threads tag:inbox > EXPECTED
 test_ruby <<"EOF"
-@q = @db.query('tag:inbox')
-puts @q.count_threads()
+puts @db.query('tag:inbox').count_threads()
 EOF
 
 test_begin_subtest "get all tags"
 notmuch search --output=tags '*' > EXPECTED
 test_ruby <<"EOF"
-@t = @db.all_tags()
-for tag in @t do
-   puts tag
+@db.all_tags.each do |tag|
+  puts tag
 end
 EOF
 
-- 
2.31.0

[PATCH 8/9] test: ruby: don't use instance variables

[Felipe Contreras]

Local variables are perfectly fine.

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

diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
index f5a8d245..30168109 100755
--- a/test/T395-ruby.sh
+++ b/test/T395-ruby.sh
@@ -12,7 +12,7 @@ test_ruby() {
     (
 	cat <<-EOF
 	require 'notmuch'
-	@db = Notmuch::Database.new('$MAIL_DIR')
+	db = Notmuch::Database.new('$MAIL_DIR')
 	EOF
 	cat
     ) | $NOTMUCH_RUBY -I "$NOTMUCH_BUILDDIR/bindings/ruby"> OUTPUT
@@ -22,9 +22,9 @@ test_ruby() {
 test_begin_subtest "compare thread ids"
 notmuch search --sort=oldest-first --output=threads tag:inbox | sed s/^thread:// > EXPECTED
 test_ruby <<"EOF"
-@q = @db.query('tag:inbox')
-@q.sort = Notmuch::SORT_OLDEST_FIRST
-@q.search_threads.each do |t|
+q = db.query('tag:inbox')
+q.sort = Notmuch::SORT_OLDEST_FIRST
+q.search_threads.each do |t|
   puts t.thread_id
 end
 EOF
@@ -32,9 +32,9 @@ EOF
 test_begin_subtest "compare message ids"
 notmuch search --sort=oldest-first --output=messages tag:inbox | sed s/^id:// > EXPECTED
 test_ruby <<"EOF"
-@q = @db.query('tag:inbox')
-@q.sort = Notmuch::SORT_OLDEST_FIRST
-@q.search_messages.each do |m|
+q = db.query('tag:inbox')
+q.sort = Notmuch::SORT_OLDEST_FIRST
+q.search_messages.each do |m|
   puts m.message_id
 end
 EOF
@@ -42,25 +42,25 @@ EOF
 test_begin_subtest "get non-existent file"
 echo nil > EXPECTED
 test_ruby <<"EOF"
-p @db.find_message_by_filename('i-dont-exist')
+p db.find_message_by_filename('i-dont-exist')
 EOF
 
 test_begin_subtest "count messages"
 notmuch count --output=messages tag:inbox > EXPECTED
 test_ruby <<"EOF"
-puts @db.query('tag:inbox').count_messages()
+puts db.query('tag:inbox').count_messages()
 EOF
 
 test_begin_subtest "count threads"
 notmuch count --output=threads tag:inbox > EXPECTED
 test_ruby <<"EOF"
-puts @db.query('tag:inbox').count_threads()
+puts db.query('tag:inbox').count_threads()
 EOF
 
 test_begin_subtest "get all tags"
 notmuch search --output=tags '*' > EXPECTED
 test_ruby <<"EOF"
-@db.all_tags.each do |tag|
+db.all_tags.each do |tag|
   puts tag
 end
 EOF
-- 
2.31.0

[PATCH 9/9] test: ruby: simplify output comparison

[Felipe Contreras]

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

diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
index 30168109..597330d3 100755
--- a/test/T395-ruby.sh
+++ b/test/T395-ruby.sh
@@ -20,22 +20,22 @@ test_ruby() {
 }
 
 test_begin_subtest "compare thread ids"
-notmuch search --sort=oldest-first --output=threads tag:inbox | sed s/^thread:// > EXPECTED
+notmuch search --sort=oldest-first --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|
-  puts t.thread_id
+  puts 'thread:%s' % t.thread_id
 end
 EOF
 
 test_begin_subtest "compare message ids"
-notmuch search --sort=oldest-first --output=messages tag:inbox | sed s/^id:// > EXPECTED
+notmuch search --sort=oldest-first --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|
-  puts m.message_id
+  puts 'id:%s' % m.message_id
 end
 EOF
 
-- 
2.31.0

Re: [PATCH 1/9] test: move test_ruby() inside the only client

[David Bremner]

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

> Not much point in polluting the main library, and also will be useful to
> modify it in tandem with the tests.
>

I can live with this change.

d

Re: [PATCH 2/9] test: ruby: refactor test_ruby()

[David Bremner]

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

> There's no point in repeating the same initialization in all the tests.
>

LGTM

d

Re: [PATCH 3/9] test: ruby: simplify MAIL_DIR check

[David Bremner]

LGTM. Although I am no expert on ruby idiom.

d

Re: [PATCH 4/9] test: ruby: simplify MAIL_DIR initialization

[David Bremner]

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

> There's no need to complicate the script passing the MAIL_DIR
> environment variable.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

The interpolation inside single quotes is slightly surprising, but OK

d

Re: [PATCH 5/9] test: ruby: simplify test_ruby()

[David Bremner]

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

> We always do test_expect_equal_file, so do it in test_ruby() directly.
>
> The only subtest where we don't (get non-existent file) can be easily
> modified.

I'm slightly hesitent since every other test ends with test_expect_*.
OTOH it is self contained within this one file so I guess I can live
with this change.

d

Re: [PATCH 6/9] test: ruby: use much more standard puts

[David Bremner]

No objection.

d

Re: [PATCH 7/9] test: ruby: use much more standard Ruby idioms

[David Bremner]

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

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  test/T395-ruby.sh | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
> index f871ddd9..f5a8d245 100755
> --- a/test/T395-ruby.sh
> +++ b/test/T395-ruby.sh
> @@ -24,7 +24,7 @@ notmuch search --sort=oldest-first --output=threads tag:inbox | sed s/^thread://
>  test_ruby <<"EOF"
>  @q = @db.query('tag:inbox')
>  @q.sort = Notmuch::SORT_OLDEST_FIRST
> -for t in @q.search_threads do
> +@q.search_threads.each do |t|
>    puts t.thread_id
>  end
>  EOF

The downside to these changes is that they make the tests harder for the
non-rubyist (i.e. me) to read. So I'm not (yet) convinced this is a good change.

Re: [PATCH 8/9] test: ruby: don't use instance variables

[David Bremner]

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

> Local variables are perfectly fine.
>

LGTM (given it works)

Re: [PATCH 9/9] test: ruby: simplify output comparison

[David Bremner]

I can live with this change.

d

Re: [PATCH 0/9] test: ruby: several cleanups and simplifications

[David Bremner]

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

> I found a lot of areas of improvement in the Ruby tests, so I decided to clean them up.
>
> With these changes the tests are now much simpler and follow more closely the typical Ruby idioms.
>
> Felipe Contreras (9):
>   test: move test_ruby() inside the only client
>   test: ruby: refactor test_ruby()
>   test: ruby: simplify MAIL_DIR check
>   test: ruby: simplify MAIL_DIR initialization
>   test: ruby: simplify test_ruby()
>   test: ruby: use much more standard puts

I have applied the first 6 patches to master.

d

Re: [PATCH 1/9] test: move test_ruby() inside the only client

[Tomi Ollila]

On Sun, May 02 2021, David Bremner wrote:

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Not much point in polluting the main library, and also will be useful to
>> modify it in tandem with the tests.
>>
>
> I can live with this change.

I'd say strong LGTM :D

Tomi

>
> d
> _______________________________________________
> notmuch mailing list -- notmuch@notmuchmail.org
> To unsubscribe send an email to notmuch-leave@notmuchmail.org

Re: [PATCH 7/9] test: ruby: use much more standard Ruby idioms

[Tomi Ollila]

On Sun, May 02 2021, David Bremner wrote:

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  test/T395-ruby.sh | 20 ++++++++------------
>>  1 file changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
>> index f871ddd9..f5a8d245 100755
>> --- a/test/T395-ruby.sh
>> +++ b/test/T395-ruby.sh
>> @@ -24,7 +24,7 @@ notmuch search --sort=oldest-first --output=threads tag:inbox | sed s/^thread://
>>  test_ruby <<"EOF"
>>  @q = @db.query('tag:inbox')
>>  @q.sort = Notmuch::SORT_OLDEST_FIRST
>> -for t in @q.search_threads do
>> +@q.search_threads.each do |t|
>>    puts t.thread_id
>>  end
>>  EOF
>
> The downside to these changes is that they make the tests harder for the
> non-rubyist (i.e. me) to read. So I'm not (yet) convinced this is a good
> change.

I am convinced that this is good change (like all the other changes
in this series). 

Every now and then I encounter ruby code, and have seen syntax like
`@q.search_threads.each do |t|` been used, so more exposure to that
syntax is good thing (for everyone (IMO))

Tomi

Re: [PATCH 5/9] test: ruby: simplify test_ruby()

[Felipe Contreras]

On Sun, May 2, 2021 at 6:02 AM David Bremner <david@tethera.net> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > We always do test_expect_equal_file, so do it in test_ruby() directly.
> >
> > The only subtest where we don't (get non-existent file) can be easily
> > modified.
>
> I'm slightly hesitent since every other test ends with test_expect_*.
> OTOH it is self contained within this one file so I guess I can live
> with this change.

The test could be named test_ruby_expect and be passed the EXPECTED
file, or something.

-- 
Felipe Contreras

Re: [PATCH 7/9] test: ruby: use much more standard Ruby idioms

[Felipe Contreras]

On Sun, May 2, 2021 at 6:07 AM David Bremner <david@tethera.net> wrote:

> The downside to these changes is that they make the tests harder for the
> non-rubyist (i.e. me) to read. So I'm not (yet) convinced this is a good change.

Yeah, but what about the rubyists? I have *never* seen the `for`
statement used anywhere. It's basically the first thing you learn in
Ruby, right after variables and objects[1].

  File.open('README').each { |l| puts l }

It also matches the notmuch_rb_threads_each () C function, which BTW
should return an Enumerator when no block is given, in order to match
other `each` methods:

    File.open('README').each # this returns an Enumerator

Being more familiar with Ruby can only improve the Ruby bindings.

Cheers.

[1] http://docs.ruby-doc.com/docs/ProgrammingRuby/

-- 
Felipe Contreras

Re: [PATCH 7/9] test: ruby: use much more standard Ruby idioms

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:
>
> I am convinced that this is good change (like all the other changes
> in this series). 
>
> Every now and then I encounter ruby code, and have seen syntax like
> `@q.search_threads.each do |t|` been used, so more exposure to that
> syntax is good thing (for everyone (IMO))
>

OK, I've applied the patch (and the others in the series). I have my
(already expressed) reservations, but I don't want to stand in the way
of progress on the ruby bindings.

d

Re: [PATCH 7/9] test: ruby: use much more standard Ruby idioms

[Felipe Contreras]

On Wed, May 5, 2021 at 5:29 PM David Bremner <david@tethera.net> wrote:
> Tomi Ollila <tomi.ollila@iki.fi> writes:
> >
> > I am convinced that this is good change (like all the other changes
> > in this series).
> >
> > Every now and then I encounter ruby code, and have seen syntax like
> > `@q.search_threads.each do |t|` been used, so more exposure to that
> > syntax is good thing (for everyone (IMO))
>
> OK, I've applied the patch (and the others in the series). I have my
> (already expressed) reservations, but I don't want to stand in the way
> of progress on the ruby bindings.

If you have reservations about the code, feel free to let me know and
I would try to address them in further rerolls of the patch series.

I'm a long term git contributor, so I'm used to several rounds of
reviews, a bit of pushback isn't going to deter me.

But at least the patches that you pushed I don't think could have any issues.

Let's see the rest.

Cheers.

-- 
Felipe Contreras