Re: [BuildStream] Some bad mistakes, lets understand them, and make this work better

[Jim MacArthur <jim macarthur codethink co uk>]

Thanks for your email, Tristan. Some commments inline, with a lot of 
snipping:

On 11/09/18 09:03, Tristan Van Berkom via BuildStream-list wrote:
> Also, especially to those of us who are more experienced: we have to be
> spending more time reviewing code than writing code; we need to set a
> good example otherwise peer review really doesn't work and ends up
> being just a checkmark that somebody else has looked at the code.
I'm glad you've said this; I've been very surprised in the past when 
people say reviews of code should take 10% of the time to create it. 
I've always felt it should be more like 100%, or more. It's much quicker 
for me to write code with a full understanding of what I'm doing than to 
understand other people's code, unless it's exceptionally well documented.

On IRC, people often request reviews by asking for "a quick review of X" 
- which puts time pressure on the reviewer. Asking "Can anyone review X? 
It is a trivial change" is a bit better.

> One thing I have to stress is, when you perform a review; first ask
> yourself these questions:
>
>    * Does this patch make the codebase simpler than it was before ?
I think this is pretty unlikely to happen unless we dedicate effort to 
simplifying the codebase. It's wonderful when a patch simplifies the 
existing code and adds functions or performance, but not common in my 
experience.
At the moment all the engineers I'm aware of are tasked with adding 
features.
>    Good luck fixing a bug with this situation, or understanding the
>    author's intent as to how this was supposed to work in the first
>    place. I was unable to fix issue 623 at all without reducing this to
>    something comprehensive first; and it took me the whole weekend.
>
>    I should add: Tiago Gomes had done some of the same refactorings
>    I did in his branch, and I would like to take this opportunity to
>    apologize again in public for having overlooked his refactors, I was
>    in a frenzy to fix 623 and didn't take the time.
I wasn't around when 623 occurred so didn't see the fallout. Even so, 
I'd suggest that working over the weekend should be strongly avoided, 
even if the bug is holding other engineers up. Most of us work office 
hours in various timezones so we cannot collaborate on work at the 
weekend. Also, you're reinforcing your role as the person who fixes 
everything when other people break it, which I don't think any of us 
want. We should be fixing issues like this as a team. Naturally, I very 
much appreciate the effort you put in.
> I should also stress that I am not worried about our team members
> ability to add regression tests with their patches and make the
> software mostly "work", I am worried about dead-ending the overall
> codebase by growing warts on it such that there is no longer any
> comprehensible design or control flow, at which point no feature can be
> added or bug can be fixed without mass rewrites.
To be perfectly honest, the codebase was never comprehensible to me. 
I've been working with it for about 8 months now and there are still 
some black boxes in there which I haven't been able to figure out yet. 
As you're about to say...

>    * As also discussed in the "Project is under heavy load" thread[3],
>      it may go a long way to maintain some architectural diagrams to
>      better guide developers through our codebase.
I think this would be very useful. I'd like to have a go at this myself, 
time permitting.

>      I personally don't have experience working on any project which did
>      this so I'm not sure how effective it can be, but we certainly need
>      to communicate more clearly how components in our code base are
>      intended to interact, and what their respective responsibilities
>      are, since we are clearly seeing that developers are circumventing
>      the architecture to complete their tasks.
>
> I'm not sure what else we can do here. I think that we need to make
> public vs private members, and single vs double underscores, more clear
> in the HACKING.rst somehow. The concept is not very difficult, and we
> only really deviate from the Python standards because we need to hide
> some methods on public classes which our core calls into internally.
>
> We cannot however solve everything with policy in the HACKING.rst,
> people have to care about concepts like encapsulation; and the
> HACKING.rst cannot become a course on how to write maintainable code.
No, it can't - but we can make some explicit decisions in there. We all 
want to write maintainable code but our approaches differ. For example, 
my approach to writing maintainable code would include transient state 
in objects when I feel it makes code more readable, at odds with your 
definition. We can debate that and a hundred other coding styles for fun 
if you like, but for the purposes of this project I'm quite happy to go 
along with a project style where I'm aware of it.

Jim