library usage

library usage

[Dimid Duchovny]

Hello,

I've tried to use the library with a simple example, 2 messages where
one is a reply to the other:
require 'msgthr'
m1 = {id: 1, subject: 's1'}
m2 = {id: 2, subject: 're: s1'}
msgthr = Msgthr.new
msgthr.add 1, nil, m1
msgthr.add 2, [1], m2
msgthr.thread!
msgthr.walk_thread do |level, container, index|
  msg = container.msg
  subject = msg ? msg[:subject] : "[missing: <#{container.mid}>]"
  indent = '  ' * level
  printf("#{indent} % 3d. %s\n", index, subject)
end

Which fails with:
msgthr.rb:9:in `block in <main>': undefined method `msg' for
#<Array:0x00007ff404841958> (NoMethodError)

Am I missing something?

Re: library usage

[Eric Wong]

Dimid Duchovny <dimidd@gmail.com> wrote:
> Hello,
> 
> I've tried to use the library with a simple example, 2 messages where
> one is a reply to the other:
> require 'msgthr'
> m1 = {id: 1, subject: 's1'}
> m2 = {id: 2, subject: 're: s1'}
> msgthr = Msgthr.new
> msgthr.add 1, nil, m1
> msgthr.add 2, [1], m2
> msgthr.thread!

msgthr.order! { |_| } # noop sort, necessary to flatten internal structures

> msgthr.walk_thread do |level, container, index|
>   msg = container.msg
>   subject = msg ? msg[:subject] : "[missing: <#{container.mid}>]"
>   indent = '  ' * level
>   printf("#{indent} % 3d. %s\n", index, subject)
> end
> 
> Which fails with:
> msgthr.rb:9:in `block in <main>': undefined method `msg' for
> #<Array:0x00007ff404841958> (NoMethodError)
> 
> Am I missing something?

Oops, i guess the documentation is currently wrong in that
order! is not optional.  But a noop block passed to order!
should work for now.

I will make it optional for the next release, and probably
allow the proc as an arg to thread! to simplify the API.

Thanks for the report and sorry for the breakage!

Re: library usage

[Dimid Duchovny]

2017-12-27 20:01 GMT+02:00 Eric Wong <e@80x24.org>:
> Dimid Duchovny <dimidd@gmail.com> wrote:
>> Hello,
>>
>> I've tried to use the library with a simple example, 2 messages where
>> one is a reply to the other:
>> require 'msgthr'
>> m1 = {id: 1, subject: 's1'}
>> m2 = {id: 2, subject: 're: s1'}
>> msgthr = Msgthr.new
>> msgthr.add 1, nil, m1
>> msgthr.add 2, [1], m2
>> msgthr.thread!
>
> msgthr.order! { |_| } # noop sort, necessary to flatten internal structures
>
>> msgthr.walk_thread do |level, container, index|
>>   msg = container.msg
>>   subject = msg ? msg[:subject] : "[missing: <#{container.mid}>]"
>>   indent = '  ' * level
>>   printf("#{indent} % 3d. %s\n", index, subject)
>> end
>>
>> Which fails with:
>> msgthr.rb:9:in `block in <main>': undefined method `msg' for
>> #<Array:0x00007ff404841958> (NoMethodError)
>>
>> Am I missing something?
>
> Oops, i guess the documentation is currently wrong in that
> order! is not optional.  But a noop block passed to order!
> should work for now.
>
> I will make it optional for the next release, and probably
> allow the proc as an arg to thread! to simplify the API.
>
> Thanks for the report and sorry for the breakage!

Thank you for the quick reply. IMHO, the best option would be indeed
to add an optional proc to thread! . If nil, it would be equivalent to
the noop order!.

Re: library usage

[Eric Wong]

Dimid Duchovny <dimidd@gmail.com> wrote:
> Thank you for the quick reply. IMHO, the best option would be indeed
> to add an optional proc to thread! . If nil, it would be equivalent to
> the noop order!.

Thanks for the feedback, I've pushed the following out and will
likely release 1.1.0 in a day or two (in case you notice any
other things wrong).

I was planning on writing something else using this, but it's on
the backburner for now; so you might be the only user at the
moment :)

https://80x24.org/msgthr.git/patch?id=d06d8c221be5b82d00da821323fb6d1889e58105
-------------8<------------
Subject: simplify API to avoid (and raise on) user errors

This fixes our API to match the documentation in making
Msgthr#order! optional.  Furthermore, the block previously
passed to Msgthr#order! may now be passed to Msgthr#thread!
instead.

We accomplish this by tracking internal state explicitly, so a
Msgthr::StateError exception will be raised when methods are
called in an unsupported order.  This internal state is reset
with Msgthr#clear.

For users who truly do not care about ordering, Msgthr#walk_thread
may be called immediately after the last call to Msgthr#add.

Thanks to Dimid Duchovny for the feedback which led to this:
https://80x24.org/msgthr-public/CANKvuDc2mkxLuh+3+WXWfMXzxK2bShNesrD5xLocGOD1RybbwQ@mail.gmail.com/
---
 lib/msgthr.rb           | 59 +++++++++++++++++++++++++++++++--------------
 lib/msgthr/container.rb |  6 ++---
 test/test_msgthr.rb     | 64 ++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 107 insertions(+), 22 deletions(-)

diff --git a/lib/msgthr.rb b/lib/msgthr.rb
index ea63731..1517f28 100644
--- a/lib/msgthr.rb
+++ b/lib/msgthr.rb
@@ -7,8 +7,7 @@
 #
 # * use Msgthr.new to create a new object
 # * use Msgthr#add! for every message you have
-# * use Msgthr#thread! to perform threading operations
-# * optionally, use Msgthr#order! to sort messages
+# * use Msgthr#thread! to perform threading and (optionally) sort
 # * use Msgthr#walk_thread to iterate through the threaded tree
 #
 # See https://80x24.org/msgthr/README for more info
@@ -20,43 +19,39 @@ class Msgthr
   # calling Msgthr#thread!
   attr_reader :rootset
 
+  # raised when methods are called in an unsupported order
+  StateError = Class.new(RuntimeError)
+
   # Initialize a Msgthr object
   def initialize
     @id_table = {}
     @rootset = []
+    @state = :init # :init => :threaded => :ordered
   end
 
   # Clear internal data structures to save memory and prepare for reuse
   def clear
     @rootset.clear
     @id_table.clear
+    @state = :init
   end
 
   # Performs threading on the messages and returns the rootset
   # (set of message containers without parents).
   #
-  # Call this after all #add operations are complete.
-  # This does not sort, use #order! if sorting is necessary.
-  def thread!
-    ret = @rootset
-    @id_table.each_value { |cont| ret << cont if cont.parent.nil? }.clear
-    ret
-  end
-
-  # Performs an in-place sort on messages after thread!
-  # This is optional and intended to be called this only after #thread!
+  # Call this only after all #add operations are complete.
   #
-  # This takes a block which yields an array of Msgthr::Container
-  # objects for sorting.
+  # If given an optional block, it will perform an in-place sort
+  # using the block parameter.
   #
-  # To sort by unique +mid+ identifiers for each container:
+  # To thread and sort by unique +mid+ identifiers for each container:
   #
-  #   msgthr.order! { |ary| ary.sort_by!(&:mid) }
+  #   msgthr.thread! { |ary| ary.sort_by!(&:mid) }
   #
   # If your opaque message pointer contains a +time+ accessor which gives
   # a Time object:
   #
-  #   msgthr.order! do |ary|
+  #   msgthr.thread! do |ary|
   #     ary.sort_by! do |cont| # Msgthr::Container
   #       cur = cont.topmost
   #       cur ? cur.msg.time : Time.at(0)
@@ -67,16 +62,40 @@ class Msgthr
   # Msgthr::Container#mid, as any known missing messages (ghosts)
   # will still have a +mid+.  However, Msgthr::Container#topmost is
   # necessary if accessing Msgthr::Container#msg.
+  def thread!
+    raise StateError, "already #@state" if @state != :init
+    ret = @rootset
+    @id_table.each_value { |cont| ret << cont if cont.parent.nil? }.clear
+    @state = :threaded
+    order! { |ary| yield(ary) } if block_given?
+    ret
+  end
+
+  # Calling this method is unnecessary since msgthr 1.1.0.
+  # In previous releases, the #thread! did not support a block
+  # parameter for ordering.  This method remains for compatibility.
   def order!
+    case @state
+    when :init then raise StateError, "#thread! not called"
+    when :ordered then raise StateError, "already #@state"
+    # else @state == :threaded
+    end
+
     yield @rootset
     @rootset.each do |cont|
       # this calls Msgthr::Container#order!, which is non-recursive
       cont.order! { |children| yield(children) }
     end
+    @state = :ordered
+    @rootset
   end
 
   # non-recursively walk a set of messages after #thread!
-  # (and optionally, #order!)
+  # (and optionally, #order!).
+  #
+  # If you do not care about ordering, you may call this
+  # immediately after all #add operations are complete starting
+  # with msgthr 1.1.0
   #
   # This takes a block and yields 3 elements to it: +|level, container, index|+
   # for each message container.
@@ -95,6 +114,8 @@ class Msgthr
   #     printf("#{indent} % 3d. %s\n", index, subject)
   #   end
   def walk_thread
+    thread! if @state == :init
+    order! { |_| } if @state == :threaded
     i = -1
     q = @rootset.map { |cont| [ 0, cont, i += 1 ] }
     while tmp = q.shift
@@ -128,6 +149,8 @@ class Msgthr
   # calling this method to avoid wasting memory on hash keys.  Likewise
   # is true for any String objects in +refs+.
   def add(mid, refs, msg)
+    @state == :init or raise StateError, "cannot add when already #@state"
+
     cur = @id_table[mid] ||= Msgthr::Container.new(mid)
     cur.msg = msg
     refs or return
diff --git a/lib/msgthr/container.rb b/lib/msgthr/container.rb
index e51507c..fbff719 100644
--- a/lib/msgthr/container.rb
+++ b/lib/msgthr/container.rb
@@ -1,9 +1,9 @@
 # Copyright (C) 2016 all contributors <msgthr-public@80x24.org>
 # License: GPL-2.0+ <https://www.gnu.org/licenses/gpl-2.0.txt>
 
-# An internal container class, this is exposed for Msgthr#order!
-# and Msgthr#walk_thread APIs.  They should should not be initialized
-# in your own code.
+# An internal container class, this is exposed for Msgthr#thread!
+# Msgthr#order! and Msgthr#walk_thread APIs through block parameters.
+# They should should not be initialized in your own code.
 #
 # One container object will exist for every message you call Msgthr#add! on,
 # so there can potentially be many of these objects for large sets of
diff --git a/test/test_msgthr.rb b/test/test_msgthr.rb
index 19cec75..b14e135 100644
--- a/test/test_msgthr.rb
+++ b/test/test_msgthr.rb
@@ -11,8 +11,9 @@ class TestMsgthr < Test::Unit::TestCase
     thr.add('c', nil, 'c')
     thr.add('D', nil, 'D')
     thr.add('d', %w(missing), 'd')
-    thr.thread!
+    rset = thr.thread!
     rootset = thr.order! { |c| c.sort_by!(&:mid) }
+    assert_same rset, rootset
     assert_equal %w(D c missing), rootset.map(&:mid)
     assert_equal 'D', rootset[0].msg
     assert_equal %w(b), rootset[1].children.map(&:mid)
@@ -33,4 +34,65 @@ class TestMsgthr < Test::Unit::TestCase
 EOF
     assert_equal exp, out
   end
+
+  def test_order_in_thread
+    thr = Msgthr.new
+    thr.add(1, nil, 'a')
+    thr.add(2, [1], 'b')
+    thr.thread! do |ary|
+      ary.sort_by! do |cont|
+        cur = cont.topmost
+        cur ? cur : 0
+      end
+    end
+    out = ''
+    thr.walk_thread do |level, container, index|
+      msg = container.msg
+      out << "#{level} [#{index}] #{msg}\n"
+    end
+    exp = <<EOF.b
+0 [0] a
+1 [0] b
+EOF
+    assert_equal exp, out
+  end
+
+  def test_out_of_order
+    thr = Msgthr.new
+    thr.thread!
+    assert_raise(Msgthr::StateError) { thr.add(1, nil, 'a') }
+    thr.clear # make things good again, following should not raise:
+    thr.add(1, nil, 'a')
+    thr.thread!
+    assert_raise(Msgthr::StateError) { thr.thread! }
+
+    out = []
+    thr.walk_thread do |level, container, index|
+      msg = container.msg
+      out << "#{level} [#{index}] #{msg}"
+    end
+    assert_equal [ '0 [0] a' ], out
+    assert_raise(Msgthr::StateError) { thr.thread! { raise "DO NOT CALL" } }
+    assert_raise(Msgthr::StateError) { thr.order! { |_| raise "DO NOT CALL" } }
+
+    # this is legal, even if non-sensical
+    thr.clear
+    thr.walk_thread { |level, container, index| raise "DO NOT CALL" }
+  end
+
+  def test_short_add_to_walk
+    thr = Msgthr.new
+    thr.add(1, nil, 'a')
+    thr.add(2, [1], 'b')
+    out = ''
+    thr.walk_thread do |level, container, index|
+      msg = container.msg
+      out << "#{level} [#{index}] #{msg}\n"
+    end
+    exp = <<EOF.b
+0 [0] a
+1 [0] b
+EOF
+    assert_equal exp, out
+  end
 end
-- 
EW

Re: library usage

[Dimid Duchovny]

2017-12-28 3:43 GMT+02:00 Eric Wong <e@80x24.org>:
> Dimid Duchovny <dimidd@gmail.com> wrote:
>> Thank you for the quick reply. IMHO, the best option would be indeed
>> to add an optional proc to thread! . If nil, it would be equivalent to
>> the noop order!.
>
> Thanks for the feedback, I've pushed the following out and will
> likely release 1.1.0 in a day or two (in case you notice any
> other things wrong).
>
> I was planning on writing something else using this, but it's on
> the backburner for now; so you might be the only user at the
> moment :)
>

Thank you very much Eric!
As a matter of fact, I did find an issue, which I'm not sure is a bug;
consider this line from the `add` method:
cur = @id_table[mid] ||= Msgthr::Container.new(mid)

If `mid` is the empty string or a string which only contains
whitespace, it's considered a valid key for `id_table`.
However, I'd expect an empty string and (arguably) whitespace-only
strings to be equivalent to nil.

Happy new year!

> https://80x24.org/msgthr.git/patch?id=d06d8c221be5b82d00da821323fb6d1889e58105
> -------------8<------------
> Subject: simplify API to avoid (and raise on) user errors
>
> This fixes our API to match the documentation in making
> Msgthr#order! optional.  Furthermore, the block previously
> passed to Msgthr#order! may now be passed to Msgthr#thread!
> instead.
>
> We accomplish this by tracking internal state explicitly, so a
> Msgthr::StateError exception will be raised when methods are
> called in an unsupported order.  This internal state is reset
> with Msgthr#clear.
>
> For users who truly do not care about ordering, Msgthr#walk_thread
> may be called immediately after the last call to Msgthr#add.
>
> Thanks to Dimid Duchovny for the feedback which led to this:
> https://80x24.org/msgthr-public/CANKvuDc2mkxLuh+3+WXWfMXzxK2bShNesrD5xLocGOD1RybbwQ@mail.gmail.com/
> ---
>  lib/msgthr.rb           | 59 +++++++++++++++++++++++++++++++--------------
>  lib/msgthr/container.rb |  6 ++---
>  test/test_msgthr.rb     | 64 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 107 insertions(+), 22 deletions(-)
>
> diff --git a/lib/msgthr.rb b/lib/msgthr.rb
> index ea63731..1517f28 100644
> --- a/lib/msgthr.rb
> +++ b/lib/msgthr.rb
> @@ -7,8 +7,7 @@
>  #
>  # * use Msgthr.new to create a new object
>  # * use Msgthr#add! for every message you have
> -# * use Msgthr#thread! to perform threading operations
> -# * optionally, use Msgthr#order! to sort messages
> +# * use Msgthr#thread! to perform threading and (optionally) sort
>  # * use Msgthr#walk_thread to iterate through the threaded tree
>  #
>  # See https://80x24.org/msgthr/README for more info
> @@ -20,43 +19,39 @@ class Msgthr
>    # calling Msgthr#thread!
>    attr_reader :rootset
>
> +  # raised when methods are called in an unsupported order
> +  StateError = Class.new(RuntimeError)
> +
>    # Initialize a Msgthr object
>    def initialize
>      @id_table = {}
>      @rootset = []
> +    @state = :init # :init => :threaded => :ordered
>    end
>
>    # Clear internal data structures to save memory and prepare for reuse
>    def clear
>      @rootset.clear
>      @id_table.clear
> +    @state = :init
>    end
>
>    # Performs threading on the messages and returns the rootset
>    # (set of message containers without parents).
>    #
> -  # Call this after all #add operations are complete.
> -  # This does not sort, use #order! if sorting is necessary.
> -  def thread!
> -    ret = @rootset
> -    @id_table.each_value { |cont| ret << cont if cont.parent.nil? }.clear
> -    ret
> -  end
> -
> -  # Performs an in-place sort on messages after thread!
> -  # This is optional and intended to be called this only after #thread!
> +  # Call this only after all #add operations are complete.
>    #
> -  # This takes a block which yields an array of Msgthr::Container
> -  # objects for sorting.
> +  # If given an optional block, it will perform an in-place sort
> +  # using the block parameter.
>    #
> -  # To sort by unique +mid+ identifiers for each container:
> +  # To thread and sort by unique +mid+ identifiers for each container:
>    #
> -  #   msgthr.order! { |ary| ary.sort_by!(&:mid) }
> +  #   msgthr.thread! { |ary| ary.sort_by!(&:mid) }
>    #
>    # If your opaque message pointer contains a +time+ accessor which gives
>    # a Time object:
>    #
> -  #   msgthr.order! do |ary|
> +  #   msgthr.thread! do |ary|
>    #     ary.sort_by! do |cont| # Msgthr::Container
>    #       cur = cont.topmost
>    #       cur ? cur.msg.time : Time.at(0)
> @@ -67,16 +62,40 @@ class Msgthr
>    # Msgthr::Container#mid, as any known missing messages (ghosts)
>    # will still have a +mid+.  However, Msgthr::Container#topmost is
>    # necessary if accessing Msgthr::Container#msg.
> +  def thread!
> +    raise StateError, "already #@state" if @state != :init
> +    ret = @rootset
> +    @id_table.each_value { |cont| ret << cont if cont.parent.nil? }.clear
> +    @state = :threaded
> +    order! { |ary| yield(ary) } if block_given?
> +    ret
> +  end
> +
> +  # Calling this method is unnecessary since msgthr 1.1.0.
> +  # In previous releases, the #thread! did not support a block
> +  # parameter for ordering.  This method remains for compatibility.
>    def order!
> +    case @state
> +    when :init then raise StateError, "#thread! not called"
> +    when :ordered then raise StateError, "already #@state"
> +    # else @state == :threaded
> +    end
> +
>      yield @rootset
>      @rootset.each do |cont|
>        # this calls Msgthr::Container#order!, which is non-recursive
>        cont.order! { |children| yield(children) }
>      end
> +    @state = :ordered
> +    @rootset
>    end
>
>    # non-recursively walk a set of messages after #thread!
> -  # (and optionally, #order!)
> +  # (and optionally, #order!).
> +  #
> +  # If you do not care about ordering, you may call this
> +  # immediately after all #add operations are complete starting
> +  # with msgthr 1.1.0
>    #
>    # This takes a block and yields 3 elements to it: +|level, container, index|+
>    # for each message container.
> @@ -95,6 +114,8 @@ class Msgthr
>    #     printf("#{indent} % 3d. %s\n", index, subject)
>    #   end
>    def walk_thread
> +    thread! if @state == :init
> +    order! { |_| } if @state == :threaded
>      i = -1
>      q = @rootset.map { |cont| [ 0, cont, i += 1 ] }
>      while tmp = q.shift
> @@ -128,6 +149,8 @@ class Msgthr
>    # calling this method to avoid wasting memory on hash keys.  Likewise
>    # is true for any String objects in +refs+.
>    def add(mid, refs, msg)
> +    @state == :init or raise StateError, "cannot add when already #@state"
> +
>      cur = @id_table[mid] ||= Msgthr::Container.new(mid)
>      cur.msg = msg
>      refs or return
> diff --git a/lib/msgthr/container.rb b/lib/msgthr/container.rb
> index e51507c..fbff719 100644
> --- a/lib/msgthr/container.rb
> +++ b/lib/msgthr/container.rb
> @@ -1,9 +1,9 @@
>  # Copyright (C) 2016 all contributors <msgthr-public@80x24.org>
>  # License: GPL-2.0+ <https://www.gnu.org/licenses/gpl-2.0.txt>
>
> -# An internal container class, this is exposed for Msgthr#order!
> -# and Msgthr#walk_thread APIs.  They should should not be initialized
> -# in your own code.
> +# An internal container class, this is exposed for Msgthr#thread!
> +# Msgthr#order! and Msgthr#walk_thread APIs through block parameters.
> +# They should should not be initialized in your own code.
>  #
>  # One container object will exist for every message you call Msgthr#add! on,
>  # so there can potentially be many of these objects for large sets of
> diff --git a/test/test_msgthr.rb b/test/test_msgthr.rb
> index 19cec75..b14e135 100644
> --- a/test/test_msgthr.rb
> +++ b/test/test_msgthr.rb
> @@ -11,8 +11,9 @@ class TestMsgthr < Test::Unit::TestCase
>      thr.add('c', nil, 'c')
>      thr.add('D', nil, 'D')
>      thr.add('d', %w(missing), 'd')
> -    thr.thread!
> +    rset = thr.thread!
>      rootset = thr.order! { |c| c.sort_by!(&:mid) }
> +    assert_same rset, rootset
>      assert_equal %w(D c missing), rootset.map(&:mid)
>      assert_equal 'D', rootset[0].msg
>      assert_equal %w(b), rootset[1].children.map(&:mid)
> @@ -33,4 +34,65 @@ class TestMsgthr < Test::Unit::TestCase
>  EOF
>      assert_equal exp, out
>    end
> +
> +  def test_order_in_thread
> +    thr = Msgthr.new
> +    thr.add(1, nil, 'a')
> +    thr.add(2, [1], 'b')
> +    thr.thread! do |ary|
> +      ary.sort_by! do |cont|
> +        cur = cont.topmost
> +        cur ? cur : 0
> +      end
> +    end
> +    out = ''
> +    thr.walk_thread do |level, container, index|
> +      msg = container.msg
> +      out << "#{level} [#{index}] #{msg}\n"
> +    end
> +    exp = <<EOF.b
> +0 [0] a
> +1 [0] b
> +EOF
> +    assert_equal exp, out
> +  end
> +
> +  def test_out_of_order
> +    thr = Msgthr.new
> +    thr.thread!
> +    assert_raise(Msgthr::StateError) { thr.add(1, nil, 'a') }
> +    thr.clear # make things good again, following should not raise:
> +    thr.add(1, nil, 'a')
> +    thr.thread!
> +    assert_raise(Msgthr::StateError) { thr.thread! }
> +
> +    out = []
> +    thr.walk_thread do |level, container, index|
> +      msg = container.msg
> +      out << "#{level} [#{index}] #{msg}"
> +    end
> +    assert_equal [ '0 [0] a' ], out
> +    assert_raise(Msgthr::StateError) { thr.thread! { raise "DO NOT CALL" } }
> +    assert_raise(Msgthr::StateError) { thr.order! { |_| raise "DO NOT CALL" } }
> +
> +    # this is legal, even if non-sensical
> +    thr.clear
> +    thr.walk_thread { |level, container, index| raise "DO NOT CALL" }
> +  end
> +
> +  def test_short_add_to_walk
> +    thr = Msgthr.new
> +    thr.add(1, nil, 'a')
> +    thr.add(2, [1], 'b')
> +    out = ''
> +    thr.walk_thread do |level, container, index|
> +      msg = container.msg
> +      out << "#{level} [#{index}] #{msg}\n"
> +    end
> +    exp = <<EOF.b
> +0 [0] a
> +1 [0] b
> +EOF
> +    assert_equal exp, out
> +  end
>  end
> --
> EW

Re: library usage

[Eric Wong]

Dimid Duchovny <dimidd@gmail.com> wrote:
> Thank you very much Eric!
> As a matter of fact, I did find an issue, which I'm not sure is a bug;
> consider this line from the `add` method:
> cur = @id_table[mid] ||= Msgthr::Container.new(mid)
> 
> If `mid` is the empty string or a string which only contains
> whitespace, it's considered a valid key for `id_table`.
> However, I'd expect an empty string and (arguably) whitespace-only
> strings to be equivalent to nil.

If msgthr were only for RFC822-like (email/news) messages, I'd
be inclined to agree with you.  However, msgthr could also be
loading messages off something like an SQL DB, where
"" and NULL are distinctly different (yet still likely a bug in
the DB design/code).  So in that case; I don't think we should
be assuming anything about the users' data and potentially
hiding bugs.

> Happy new year!

Same to you :)  I'm looking forward to having more time to work on
mail-related tools in 2018.