[boost] Review quality (was: [stacktrace] review (changing vote to NO))

Subject: [boost] Review quality (was: [stacktrace] review (changing vote to NO))
From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2016-12-25 22:18:14

On Mon, Dec 26, 2016 at 3:58 AM, Vladimir Batov
<Vladimir.Batov_at_[hidden]> wrote:
> On 2016-12-26 10:41, Andrey Semashev wrote:
>>
>> Acknowledging the need for a particular functionality is not the same
>> as accepting a particular implementation.
>
> Put bluntly users do not care about implementation.

Please, don't speak for others. I, personally, very much care about
implementation, and I can see other reviewers did take a look at the
implementation as well. I think, making judgment on the library
quality without regard to the implementation quality is irresponsible
and short sighted. That kind of approach tend to result in "5 minute
hack" solutions of obviously poor quality. But that's just my opinion.

> They care about
> functionality provided. I advocate giving the lib a "foot in the door". That
> and far better feedback from the users will be a strong incentive for the
> author to keep working and improving the lib. You seem to prefer
> all-or-nothing approach. I believe it's unrealistic and bad. You might
> disagree.

First, nowhere in my message I advocated for "all-or-nothing
approach". What I advocated for is a fair review, outlining all ups
and downs of the library, and if the reviewer feels downs are
significant enough, he would be right to vote for rejection.

Second, it's not like the library does not exist before it is
accepted. Users and the author have every opportunity to try the
library in the field, if they want to. There is blincubator.com as
well.

>> By
>> turning a blind eye on a design flaw of a presented library you're
>> doing a disservice to the library author and the library users.
>> Instead, by giving constructive feedback (positive and negative),
>> especially if supported by real world experience, you help the library
>> improvement. In fact, this (the feedback) is the most valuable reward
>> for the author from the review. And let me say, there's nothing
>> humiliating in having a library rejected during a review.
>
> Well, I do not get scolded often these days. So, it is quite "refreshing" to
> learn that I am "turning a blind eye on a design" and "doing a disservice".
> I thought I was merely expressing my humble opinion... but probably I got it
> all wrong.

Well, maybe you shouldn't have taken my message personally then. It
wasn't meant to scold anyone. If anything, it was written with the
intent to provide another (mine) perspective on what a review is and
object to your notion of library review as being a humiliation. The
last part, actually, was kind of offending from your side and what
triggered my reply in the first place.

> As for the feedback, then I do not see voting something useful "no" to be
> much of a feedback. Indeed, the author does get initial feedback during a
> review. However, not all of that feedback is constructive. Some valid, some
> subjective, some capricious. After it's voted out the feedback is no more.

The review entails a discussion. Some reviewers get convinced. Some
review points get refined, and the author gets convinced. Sometimes
review votes get changed. And it's not like the votes are final - the
actual decision is taken by the review manager, and he can weigh the
points made in reviews and discussion at his discretion. And it's not
like the library disappears after the review. If it's interesting and
useful, it'll have its users, and they will no doubt provide feedback.

> Real valuable feedback will come when the lib is in Boost. People'll start
> using the lib (I would) in their real projects asking for this and that.

Real world usage produces valuable feedback, no doubt about that. But
Boost acceptance is not a pre-requisite for real world usage.

>> There are things that can be fixed post-review.
>> ...
>> There are other things that cannot be fixed easily and would probably
>> require changing library design. Those changes often affect library
>> API and usage patterns, which warrants the need of a new review of the
>> reworked library.
>
> Yes, all that sounds wonderful... on paper. In real life as a user I'll take
> what's available first. Then, if that is improved in the next version, I'll
> take it gladly. If design changes and gives me more or fixes something, I'll
> accept that.

I doubt you'd be so easy to accept the changes if that required you to
rewrite half of your code. :)

> I do not remember Spirit transitioning from V1 to not
> backward-compatible V2 being re-reviewed. I can be wrong though.

I don't remember the review either. But Spirit v2 was added in
addition to Spirit Classic (aka v1), which is still available in
Boost. It was more like a new library was added. Should it have been
reviewed? In my opinion, at least a mini-review should have been
performed. Luckily, Boost.Spirit dev team has rich experteese in the
domain and a lot of experience gained with v1, so v2 was a success.

>> In those cases it's better to reject the library so
>> that the new, improved version is presented later.
>
> Again, wonderful.. in theory. The reality IMO is different because 1)
> "improving" is better with real user feedback that you deny to the author by
> voting "no"; 2) "later" might never come as the review process is quite
> exhausting/draining; not everyone wants to experience it again; you might
> say "too bad for him"; I'll say it's bad for the user as well as they end up
> with your good intentions and no library; 3) "later" might be too late as by
> that time the user'll have something else in place.

Again, it's not like the library doesn't exist outside Boost. If it
fits you, go right ahead, so 1 is not true. But Boost does set a
certain bar of quality, and the review process exists precisely to
maintain that bar. And I'll take your 2 and 3 any time if it results
in a high quality library in Boost.

>> Consider also that whenever a library is accepted into Boost, the
>> matter of backward compatibility appears. If the accepted library is
>> somehow seriously flawed, that flaw may be difficult and painful to
>> fix in the future.
>
> "difficult", "painful", "backward compatibility"... Come on. I am sure
> you've been in the industry for a while, have "survived" many
> upgrades/updates, etc. We all know it's not an issue. We update, adjust,
> move on.

Let's just say it can be an issue.

On Mon, Dec 26, 2016 at 3:54 AM, Robert Ramey <ramey_at_[hidden]> wrote:
> On 12/25/16 3:41 PM, Andrey Semashev wrote:
>>
>> Consider also that whenever a library is accepted into Boost, the
>> matter of backward compatibility appears.
>
> How so? Certainly there is no requirement that boost libraries support older
> versions of C++ and/or compilers - and indeed many don't.  I don't think I
> see what you're referring to here.

I think you misunderstood. I wasn't speaking of C++ versions backward
compatibility. I was speaking of the library backward compatibility.
AFAIK, making backward incompatible changes is still considered an
undesirable practice.

>> If the accepted library is somehow seriously flawed,
>
> Then of course it should be rejected.  Of course reaching a concensus as to
> whether it's flawed might not be so easy.

Right, that was my point.

>> [1]: http://www.boost.org/community/reviews.html
>
> I checked this page and don't see where it says anything relevent to your
> points.

Maybe you missed this section:

http://www.boost.org/community/reviews.html#Comments

<quote>
Your comments may be brief or lengthy, but basically the Review
Manager needs your evaluation of the library. If you identify problems
along the way, please note if they are minor, serious, or
showstoppers.

The goal of a Boost library review is to improve the library through
constructive criticism, and at the end a decision must be made: is the
library good enough at this point to accept into Boost? If not, we
hope to have provided enough constructive criticism for it to be
improved and accepted at a later time. The Serialization library is a
good example of how constructive criticism resulted in revisions
resulting in an excellent library that was accepted in its second
review.
</quote>