Thanks for the detailed review. Replies inline!
Stephen J. Turnbull wrote:
Pranjal Yadav writes:
> Blog: prany.github.io
> code: giitlab.com/godricglow/mailman
Sorry about the long brain dump. Pick some of the issues to address
now and save the rest for later.
I took a look at your blog. First I reviewed your proposal blog from
April 29 (http://prany.github.io/gsoc15.html), and had the following
thoughts.
1. I don't understand why the "X-Original-To" header field is checked
after "To". I imagine there is a reason, but as this header is
non-standard, something needs to be said about its presumed
semantics and when you would expect it to appear, as well as what
MTAs implement it (I don't think Mailman itself does, at least not
in our upstream version).
I agree with you, I was checking for "X-Original-To:" due to some
confusion, I
read /mailman/config/schema and misunderstood the last part where it says
subsequent methods will be re-written with second headers and adding to
the confusion was systers implementation which too checked for both. will
check for "To:" in the header.
3. I'm not sure about the wisdom of having a separate named pipeline
for dlists. I agree with you that the actual pipeline should
include different handlers rather than trying to incorporate the
dlist logic in existing handlers. (Of course somebody may want to
refactor to exploit commonalities in the code, but IMO your best
strategy is to copy the whole calculate-recipients and decorate
Handlers and modify them as convenient.) However, Mailman already
allows per-list (2.x allowed per-message! dunno if 3 does)
pipelines.
So I'm thinking that maybe we should put the standard pipelines in
variables (a sort of pipeline registry), and *always* attach a
pipeline attribute to each list. To prevent a list from changing
the global objects for everybody, we could make pipelines tuples
rather than lists. (We would need to run that by Barry first,
though.)
I'm not sure how this would work so I will need a bit more explanation for
this comment. AFAIK all mailing lists are attached with a pipeline from
Base pipeline, Virgin-pipeline, Owner-pipeline or Posting-pipeline, mostly
all messages attach to the 'built-in' or simply default-posting-pipeline
initially. So how exactly should I create those registries from pipelines?
Adding another attribute to the mailing lists will be easy but replacing
lists as pipelines tuples is a pretty complex thought. We'll run it with
Barry
as you said and then I will post the necessary changes.
4. Since Thread.base_message_id is supposed to be a UUID, that (or a
hash) would be a reasonable primary key for the Thread. This
would lead to an interesting extension that any message could
become a new dlist. Don't know if that would be useful, but it
might be (often threads split).
Now, about the progress report blog of May 31
(http://prany.github.io/threads-for-dynamic-sublists.html). First a
few stylistic comments.
5. You start by saying that messages "are not kept in a structure."
It's not obvious what you mean by that. Obviously there are
message and metadata classes defined in Mailman. I guess you mean
that you want a history of the messages which might simply be a
list of message-ids, or their hashes. However, your Thread class
in the proposal doesn't seem to keep any history.
By "structure" I simple meant any form of order/arrangement that can
be achieved, using history is one good way of doing so. However I simply
planned to make threads to do so.
6. I disagree with you about the use of the word "conversation."
A *conversation* is an ambiguously defined collection of messages
that have various commonalities -- a time span, a topic, and a
group of participants, at least.
A *thread* is a collection of messages that can be arranged in a
DAG (or even a tree) by using the reply-to relationship (which may
be implemented using the In-Reply-To and/or References fields, but
I'm referring to the human concept of "reply" here).
A *dlist* is a collection of messages defined by having a
particular mailbox among their addressees.
As I see it, your goal is to *implement* conversations (or maybe a
better word is "facilitate") using d-lists. I have no objection
at all to using conversation in that sense. However, it's
important to recognize that what a human thinks of as a
conversation is quite different from what a machine can implement
as a thread or a dlist.
Also, while you have the right to use whatever definitions you
want, I think most people who are well-versed in email semantics
and implementations will agree with the definition of thread I
gave, and also agree that dlists are something quite different.
That means you will have a difficult time communicating if you use
the words "conversation", "thread", and "dlist" as rough synonyms.
I strongly agree with the definitions and I accept misusing the
term "conversation" which I wrote to convey an idea as to how
threads are understood in day to day life. I will make sure I don't
confuse with all these terms again.
7. I don't understand what you mean by "to make threads which are
meaningful rather than grouping them visually." I sort of
understand what you mean by "visual grouping", but in fact all
MUAs I know of also implement navigation based on the thread
structure (normally with a depth-first traversal, and the visual
display also corresponds to that order). On the other hand,
"meaningful threads" has no particular meaning to me.
8. Under "Requirement" you write
[W]e aim to use this concept [of threading] only for those mailing
lists which are initially 'dlist_enabled' or simply saying dynamic
list enabled via mailing list object's attribute.
but as described in 6, threads are quite a different concept from
dlists.[1]
Thanks for pointing this out, I will use above definitions to answer this.
*Dlist* is a collection of messages defined by having a particular mailbox
among their addressees, so when I say a list should be dlist enabled, its
merely a check that the list is good to be a Dlist, now the collection
of new
messages inside this newly formed Dlist will be arranged in an order using
Reply-To relationship and thus forming a *Thread*.
9. Under "Code" you give a simple list of modules to modify, but I
think you should describe in more detail what you propose to do.
I will include relevant part of the code with details.
10. The link to your repo is broken! (It's prefixed by your blog for
some reason.)
Some comments about the code (I'm looking at "git diff master dlist").
11. Throughout, I think you should use "dlist" rather than "thread"
for naming classes and other identifiers.
Agreed and changed!
11. In dlist.py, your __all__ variable is wrong, I believe (the dlist
module has no dlist attribute).
Thanks again for pointing this out , I will not repeat this.
12. class DlistCommands probably should derive from the main Mailman
commands class.
13. DlistCommands.__init__() should have an else: statement that gives
a useful error message. Ditto the "ignored" part of __iter__().
I haven't yet started working on "Errors" and that is on my list for
this week
so I think this won't be an issue next time.
14. In threads.py, the stub methods in class IThread should raise
NotImplemented or something like that.
Same as above.
15. In message.py, the decision to add msgdata as an attribute to the
Message model really needs justification, since the original
design carefully separates the two.
Looking into this, I had no clue about the original design and I should
be more careful with what I choose to use, lesson learnt.
16. Isn't the assignment to thread_id dead? Also, use .get() here,
not try ... except. And *never* use a bare except, not even in
very early prototype code. Defining self-documenting Exceptions
is trivial. The problem is that msgdata[] could invoke arbitrary
code which could raise things you can't imagine.
Sure, I will use .get() here, I used a bare except as this was an early
prototype which you correctly pointed out.
17. In lmtp.py you're going to need to deal with dlist names, as well
as +new and friends.
Overall I think it's a reasonable first cut, but some things seem to
be a little careless (like the __all__ lists).
Footnotes:
[1] I actually don't think implementing threads as such would be very
difficult. Simply keep a history of Message-IDs (or their hashes) and
allow users to "subscribe" to the replies to arbitrary messages (and
to replies to the replies, of course), alternatively unsubscribe (what
in a traditional MUA would be "killing" the thread). The hard part
would be the UI, since most users would not be capable of, let alone
happy with, specifying message IDs to commands. I do have some ideas
about that, though.
_______________________________________________
Mailman-Developers mailing list
Mailman-Developers@python.org
https://mail.python.org/mailman/listinfo/mailman-developers
Mailman FAQ: http://wiki.list.org/x/AgA3
Searchable Archives:
http://www.mail-archive.com/mailman-developers%40python.org/
Unsubscribe:
https://mail.python.org/mailman/options/mailman-developers/archive%40jab.org
Security Policy: http://wiki.list.org/x/QIA9
