GNOME.org
 

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

Hi Jim,

Thanks for chipping in here :)

On Wed, 2018-09-12 at 11:39 +0100, Jim MacArthur via BuildStream-list wrote:

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.


Right, to clarify; I don't expect to be able to add features and reduce
complexity all the time, although interestingly it can sometimes
happen.

Rather, I want to express that people should be considering this when
reviewing patches, and be in this mindset. Whether the code actually
works is much less important, that can be fixed and regression tested.

When it comes to a feature, can more code sharing happen sensibly ?

Or are we code sharing something inappropriately, such that we achieve
less LOC at the expense of also reduced readability ? (too much
implicit things going on behind the scenes causing a codeblock to not
be readable ?)


   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.


Maybe.

The fact is that in recent times, I have been less and less capable of
actually working on code changes myself at times when I overlap with
most other developers on the project, as such I end up only being able
to be productive (code wise) on the weekend.

Hopefully this can improve with better guidelines and architecture
diagrams and such, as discussed in Sander's replies.


 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.


You make an excellent point here; I also don't want that.

I sort of stumbled into the mess in the weekend while only having the
intention of fixing 623 - I found it impossible to sensibly fix without
the refactoring of various things, which I took note of along the way.

I don't want to be the one who fixes everything when other people break
it, I've had that experience over 10 years ago and had to work through
my own frustrations as a result, it's unhealthy.


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.


Yes, I'd also like to contribute as much as I can.

It's going to be trying to find time to improve the HACKING.rst and get
diagrams, but I think it's something we'd all like to see happen.


     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.


I won't reiterate much of the same things I've replied to Sander, and
just say that I completely agree, we need to clarify these practices in
documentation.

Cheers,
    -Tristan
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]