Re: library usage

[Dimid Duchovny <dimidd@gmail.com>]

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