🚀 go-pugleaf

blueman <NOSPAM@nospam.com> writes:
> Uday S Reddy <uDOTsDOTreddy@cs.bham.ac.uk> writes:
>> blueman wrote:
>>
>>>
>>> Quick test shows it works!
>>
>> Perhaps it does.  But I already added a fix a day or two ago, tested
>> it and committed to the repository on launchpad.  Unfortunately, this
>> to-and-fro hasn't contributed all that much to the problem, after the
>> initial discussion.  There are some 20 odd messages in this thread.  I
>> don't think this mode of communication has been very effective.
>
> Personally, I find that comment unfair. I have:
>        - Identified multiple show-stopper bugs in the routine
>        - Coded bug fixes
>        - Suggested and discussed extensions to make it usable on
>          multiple messages rather than running them together
>        - Coded the extension
>
> I have shared my code the whole way through and have accepted input from
> you and Julian which ultimately made it better.
>
> At the same time, you mention making fixes which you haven't shared here
> and which I can't seem to even find on launchpad (I am looking at
> http://bazaar.launchpad.net/~reddyuday/viewmail/8.1/changes) But based
> upon your allusions to rewriting code and adding variables, it seems
> like you may have done something more convoluted than necessary which
> could have other unforseen consequences (indeed all that was necessary
> was removing two words and quoting a third).

OK - I found your revisions.

The way you implemented it is really just a kludge which adds an
additional unnecessary variable vm-message-include-separators. 

Moreover, as I metioned in one of my earlier "unproductive" posts, your
method actually ends up adding an extra newline to each message (and
hence doesn't do what you claim it does) unless the user also remembers
to change the value of vm-pipe-message-to-command-end from "\n" to nil
-- which is hardly obvious and which you don't document. This also makes
me wonder how well you tested the function because if you had done the
minimal test of diff'ing a pipe to 'cat' of all messages in a folder
against the original folder contents you would have noticed this
difference and probably not have coded the change this way.

Additonally, you added a comment to the vm-pipe-message-to-command
function whereas the real need for the separators is in the
vm-pipe-messages-to-command function where you are combining multiple
messages in one pipe. In fact, Robert's
vm-pipe-messages-to-command-start/end variables *only* apply to the
vm-pipe-messages-to-command function.

Finally, your method doesn't allow for adding the "From_" line in the
case where you are just looking at the headers (prefix-arg 16 or 64) and
there are use cases where you might want to see the From_ and From:
lines when looking just at the non-body part of messages.

Also if you look through the "20-odd" messages, the first half were
spent productively generating the solution that I ultimately coded
(which you yourself acknowledged "Indeed, it is nice!") -- yet you
didn't include the fix in your patch but instead added an inelegant
kludge. 

And the second half of the messages were devoted to fixing the actual
show-stopper bug in the code which causes it to crash when you supply
either of the optional arguments. However, despite my supplying the bug
fixes multiple times on this thread (and in my bug report) you have not
added it to the code. Indeed I don't know how you can claim to have
properly tested this function without checking the optional variables
given that they account for much of the code complexity and thus deserve
to be tested.

So, yes this thread is a waste -- but only because you don't seem to
have listened to any of the productive suggestions included in the
thread... Even worse, in my opinion, your fix added an inelegant and
arguably broken kludge to an otherwise elegant function and to add
insult to injury the vm-pipe-messages-to-command function is itself
still fundamentally broken.