From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS3215 2.6.0.0/16 X-Spam-Status: No, score=-3.1 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.0 Received: from mail-ua0-x22c.google.com (mail-ua0-x22c.google.com [IPv6:2607:f8b0:400c:c08::22c]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id 19F5E1F406; Sun, 31 Dec 2017 13:07:20 +0000 (UTC) Received: by mail-ua0-x22c.google.com with SMTP id 34so13404201uav.5; Sun, 31 Dec 2017 05:07:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=RyUVs/4sxq53DNZJ7LTS+znaU45vesRB/CgFVvD0awY=; b=H6pkCUurJje5THsStqkiT0pb/GnUrlyu5rY0kqQ5o4J6rrdiEkweyAFuwXYli7paLt EWxkCbgVxvLSKhxYoFcqbNhM3TreT393dx+CGQwIpgLuFu7rt8dhIwoSA7oALmNlyIPt zE4qDZoaipIblLc95DCCqwEgBzTOxltZxa/Qh+4NMX0J44ZgjhEH9vlwzyy+sxbJMgBl efwM96Xez+uwSOko+g/G/EFkrgzJaNvzXv2saDJVNZEm42gDiqBqm0et3Xvfh5E5pvqG t/L/HaungZgWOjzXyKDTGhtEAh0lXb667YSPshOyg7C1k/vmM/UTwppMBxZGFJ89rpBJ lUrg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=RyUVs/4sxq53DNZJ7LTS+znaU45vesRB/CgFVvD0awY=; b=BDcJZ/Iqz5igv0b+NUtZWeLQdy54GVMKS2fHsu1wDVGZRcmKOi7kacirujfm78v+LH VlRzwPzBveJLXYtHcEVwYahlMkFZHX8VypRG0fY7NPirg+uTPaMK1QQxemHb88gT3lK6 Tk7iolR2mm8ekuE0dWmtmQiqJfii1VnF85LFGLUSM1vCG5p7qcsfOka4y2QfADOyGrlC 7uTOlrYS6NpckFGDbU/Wdkb3jXl7KtxglDk+r4ctWnPL1Du9KBCacruOGb6YRZsC9IVz sfNLq3jRdEnhodWGGudO56nSdUsBOQ0fzL06xv8GjNizRrsWvyPoOTe0uSeJLPoNr965 d6lg== X-Gm-Message-State: AKGB3mL4TsZx0UjWp4X7Tz+oQPb9vPOzp1AIW+SbdbZ4nxGBPFo0Xx6J R8g/YT6gm26lyk1FOsBHx6Pf3qs6gcxP81Ugcv4OSQ== X-Google-Smtp-Source: ACJfBovny68wU0KbGqvg+H+JYI3WbNCy0HlAYoX3ycJp3kKhoBXW6bNE6cxrVYcsW6eEeoDtt8I4egCbaKZcxOxYv94= X-Received: by 10.159.37.245 with SMTP id 108mr38799583uaf.190.1514725638604; Sun, 31 Dec 2017 05:07:18 -0800 (PST) MIME-Version: 1.0 Received: by 10.159.37.35 with HTTP; Sun, 31 Dec 2017 05:06:38 -0800 (PST) In-Reply-To: <20171228014329.GA15938@starla> References: <20171227180157.GA10868@starla> <20171228014329.GA15938@starla> From: Dimid Duchovny Date: Sun, 31 Dec 2017 15:06:38 +0200 Message-ID: Subject: Re: library usage To: Eric Wong Cc: msgthr-public@80x24.org Content-Type: text/plain; charset="UTF-8" List-Id: 2017-12-28 3:43 GMT+02:00 Eric Wong : > Dimid Duchovny 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 > # License: GPL-2.0+ > > -# 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 = < +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 = < +0 [0] a > +1 [0] b > +EOF > + assert_equal exp, out > + end > end > -- > EW