Follow-up on code-review process and merge request refinement

The following discussion from !41 (merged) should be addressed:

• @pzejdl started a discussion: (+4 comments)

  Following the discussion we had in person, I would really prefer to have this in a separate MR. It is much easier to review short MRs than a big one.

• @dinyar commented:

Hi,

I don't want to necessarily reopen the discussion here, but I wonder if we should weaken the "rules" a bit. My reasoning is that (while I agree with Petr that MRs should be as small as possible if we want to move fast) I don't think anyone will have the capacity to explicitly go through the code and make such minor changes explicitly in the near future, so I propose that code changes that, e.g. remove a compiler warning are allowed to go in "unrelated" MRs.

How about we allow changes that don't change functionality, but either

1. Remove a compiler warning
2. Move us towards adherence to the coding style
3. Something else?

can go in any MR? Maybe with the added caveat that they should be one liners or at least locally contained? I see this somewhat connected to the concept of the refinement code review¹ which I quite like.

I'm happy to discuss this on Tuesday maybe or over coffee another time so we don't force all other meeting attendants to listen. :⁠-⁠)

Cheers, Dinyar

1. https://martinfowler.com/bliki/RefinementCodeReview.html

assigned to @pzejdl, @dinyar, and @meschi

mentioned in merge request !41 (merged)

changed the description

Hi @dinyar,

I think it is fine to discuss this and to find some acceptable solution for everybody. Probably better here, in a separate issue.

My point(s) of view: I'm trying to take these code reviews seriously. I try to understand what the ideas behind them are, what the code does, suggest possible directions or solutions, or find a solution based on what other people did or what the C++ community recommends. All in a hope that we all can benefit from a (hopefully) better code and mainly that we don't have to return to the code in the future because something doesn't work as expected because something was overlooked. Or at least not too often

Personally, I prefer to focus on a single topic or functionality. I found that non-related changes are very distracting to me. I.e., I don't want to think about them when I need to stay focused on a particular topic. Or maybe I'm getting old ;)

While some occasional changes or local refinement are inevitable and I do them as well, for cumulative changes I would prefer to have a dedicated MR. Since these changes are usually short but sometimes many at various places, all of them can be accumulated in a single dedicated MR, having some generic name like "code cleaning..." or so.

Obviously, there is no real boundary between occasional changes and "cumulative" changes, so common sense must be used. Probably depends on the MR. If it is a small and simple one then it is not a problem to have more of these local refinements. But if we have a large MR or a complex one that brings a new functionality then (I would prefer to) limit them.

About the changes towards the coding style: These I would really prefer to have in a dedicated/separated MR because these changes do inflate MRs, in particular, the changes tab. We had a few examples in the past already and in the changes tab, it is very difficult to see the changes for the new functionality because it is hidden in many code style changes (that do not change the functionality). Reviewing separate MR with (only) code style change is simple since there is basically nothing to review.

To sum up, ok, I'm also very selfish: The code review takes quite some time, often takes time that can be used somewhere else; Therefore, and despite what I wrote earlier, I don't want to spend more time on the code review than is really necessary.

Now we can go and have coffee... :)

Best regards, Petr