Per Bothner <xxxxxx@bothner.com> writes:

> (1) When you contact the author of some code, you will not endear
> yourself or win friends by starting by explain that his/her code is
> "over-complicated", "inconsistent", "wrong", and buggy.  That is not a
> smart way to start the conversation, unless your goal is to piss
> him/her off.

I'm sorry that I've offended.  I really didn't mean to.

At the same time, I'm pretty clueless on how I could have better
expressed myself.  Maybe I'm just horrible at communicating.  While some
of the terms I used are subjective, they were more or less based on
concrete issues I could identify with the code, like the inconsistency
between consing vs. quoting (which lead to a bug), the complexity of
having alternative definitions of things via cond-expand (which lead to
a bug), etc.  It's also a fact that it doesn't conform to the
specification.

All of that isn't a challenge on anyone's competency at all.  I write
utterly horrible code sometimes, for various reasons, and when I realize
it (which might happen late, too) I happily call it what it is and
discourage people from using it.

> (2) Most maintainers like for each fix or re-write to be submitted
> separately, with separate justification.  A mega-patch with a lot
> of unrelated changes is hard to evaluate.   A complete re-write even
> more so.
>
> I suggest you read:
>
> http://people.redhat.com/rjones/how-to-supply-code-to-open-source-projects/

I had huge hurdles with figuring out the code while I was working on it,
and while part of it seemed intrinsic to the complexity of the task,
part of it was clearly (to me) due to the style of the code.

If others disagree that this is a serious problem, then I guess there's
nothing to do.

> (3) If the code is non-idiomatic or over-complicated there might be
> reasons for that.  It's a good idea to ask about those reasons before
> trying to simplify the code.  Specifically, the code was written
> to be maximally portable, only requiring R%RS plus SRFI-0, while
> also supporting useful optional features, enabled by cond-expand.
>
> Some aspects / reasons for the complexity:
>
> (a) define-record-type was not in R5RS, so could not be assumed
> when the code was originally written in 2005.
>
> In 2015 it may be reasonable to simplify the code by assuming
> define-record-type is available.
>
> [...]
>
> (c) R5RS has no standard exception handling mechanism.
> At this point it might be reasonable to assume a common
> subset of the R6RS/R7RS exceptions.

Indeed, I should have clarified that for a large part it's merely
modernization what my variant does.

> (b) There is no standard module system we can reasonably
> assume.  That is still the case.

The main body of the code (in the ".body.scm" file) assumes no module
system.  It uses R7RS in terms of features, which is fairly portable.

> (4)
>
>> One might say that the best way to move forward is identify the actual
>> bugs in it and solve them, not touching the rest so as not to introduce
>> any new possible bugs.
>
> Yep.
>
>> But for the time being, I have little motivation to go that route.  It's
>> mentally straining for me to go through this type of code.  I wish to
>> allocate my time and energy to other things.
>
> That is your choice.  It is also my choice to reject your submission.

OK, that's unfortunate.

> (5)
>
>> - It doesn't attempt to record line numbers itself, since test
>>    expressions are named anyhow (the expression itself is printed when
>>    the name is omitted), so I think it's an insignificant feature.up
>
> I disagree.   Expecting people to come with a unique name for each test
> adds a needless hurdle to test-suite development.  In many work-flows
> such a name is just useless noise.
>
> In other words, reporting line number of tests (especially failing tests)
> is a valuable feature.

I agree it's quite convenient when it's there, but I found the trade-off
between the offered convenience and the complexity added to the code to
be in favor of removing the feature.

(By the way it might be a good idea to report the whole "path" of a test
when it fails.  My implementation doesn't do so either so far, and I
realize this leads to problems in test suites with repetitive sub-tests
with identical names, under different sections.)

>> - The output it prints by default is arguably nicer.
>
> Perhaps.  But it's a change, and one that needs to be justified.
> I'm used to the current output, and I like it.
>
>> - It doesn't litter your working directory with a log file you didn't
>>    ask for and which contains no more than the standard output.
>
> However, in my implementation the log file contains a lot more
> information than standard output.  It is very useful in my experience.
>
>> It's only been tested moderately by me but I'm going to go on a limb and
>> say that I'm more confident in its overall bug-free-ness than the old
>> implementation, because clean code goes a long way.
>
> The innocence and optimism of the young ...
>
> As a general rule, old well-tested ugly code has fewer bugs than
> clean freshly-written code.

Well, it clearly wasn't very well tested, and I can understand ugliness
resulting from age of code but a good portion of the ugliness here was
simply bad code style.

Again, this is not a challenge on anyone's competency.  Bad code
happens, and I'm not going to pretend that it's not bad because of who
wrote it or for how long the code existed.

>> The promised list of issues for if you say no, and just want to fix them
>> in the existing implementation:
>
> I will look at these more closely.
>
>> In my opinion, most of these bugs could have been avoided if the style
>> of the code were more disciplined and simple such that it puts less
>> cognitive load on the programmer.
>
> See (1) above.

I don't use any of the words I use as slander.  I believe I made a
reasonable job of pointing out concrete issues with the code's style and
how these lead to actual bugs.  Do we have any practical empiric
measures of code cleanliness or cognitive load on the programmer?  Maybe
not.  Should we therefore ignore these issues?  I don't think so.

Kind regards,
Taylan