It’s my understanding that we want to improve the quality of submitted code, and that we want to do clearer design and motivation for new features. Here is my proposal for a little addition to the Code Review wiki page. Let me know what you think of this propossal; if you like it, click the heart. If you don’t like it, please explain why, and preferrably include a concrete way in which the process description can be improved.
The Process
Before adding a new feature (even a small one), it needs to be designed. As a minimum, the patch description should include the following where appliccable:
Description of the problem that should be solved.
List of alternative solutions, and a motivation as to why these are undesirable.
Description of the proposed solution, and a motivation as to why this is the best solution.
Limitations of the proposed solution.
Mock-up of the proposed user interface and a description of how users are going to interact with the new feature.
These ingredients don’t have to be full essays. A few well-written sentences can be very illuminating as well.
The order in which these are listed is also relevant, as it helps to guide the reader towards understanding what the patch is doing (although I think depending on the exact text, swapping 2. and 3. could be clearer). To give a counter-example, starting a patch description with “This drop-down menu allows the artist to…” can be confusing, as the reader doesn’t know anything about the underlying problem yet.
In order to not add more copy pasting. This can also be done in a separate design task and relate the patch and the design task. I personally use the patch description as a starting point of the commit message. Along the way of the patch review I continuously update the patch description so I don’t need to think the last minute what this patch was all about.
Sure. Also for an added CLI argument for Blender I wouldn’t expect a screenshot of that argument in use
When there is a design task, often it’s a bigger thing that might get committed as separate patches. In that case the individual patches should IMO still follow the above outline, as it’s then even more important that it’s clear what the patch is doing, and why, and just referencing the big overall design task won’t be enough for that. Also I would copy & adjust the relevant description to the commit message, so that it can be understood (and with that I mean including understanding the motivation behind the commit) isolated from tasks in Phabricator.
Along the way of the patch review I continuously update the patch description so I don’t need to think the last minute what this patch was all about.
My main concern is whether this would introduce additonal bureucracy and formalistic approach, pulling attention from the actual issues at hand to checking whether comments follow some in the end arbitrary templates and rules to the letter.
Edit: To elaborate, I basically suggest, that ‘patch review’ that purely focuses on checking these or some other, essentially, documentation guidelines, without addressing the substance of the issue, should not be considered proper review.
I’m assuming this only applies to big feature patches, for a run of the mill bug fix review, i don’t see how it’s needed for a dev to list all the ways fixing the bug did not work.
With ‘alternative solutions’ I don’t mean ‘failed attempts’ Often there is more than one way to cuddle a cat (the regular saying is too macabre for me), though, and each has their merit. Of course if there is really only one way to fix the bug (for example a missing MEM_free() call causing a memory leak) this isn’t necessary. For most features, though, it helps to know what alternatives were considered and to understand why these weren’t chosen.
Something similar can be applied to movation as well. A feature that’s implemented by a developer and just plonked onto the patch tracker will be perceived quite differently from a feature that’s demanded by every animator on the planet. By simply mentioning the fact that it’s widely demanded by group X, and adding a link to a forum or something where the feature was discussed and makes that claim visible, that changes a lot.
I don’t think it will. I think this is an essential step in getting a more “think first, code later” approach. I think that it’s impossible to value code at all, unless you know the background. You have to know which problem is solved in order to assess the correctness of the solution.
Every developer probably thinks of various way so solve a certain problem, so writing these considerations down should be easy. Just a line or two like “solutions B and C may seem like reasonable approaches on the surface, but do not take case X or Y into consideration” might be enough.
Of course we shouldn’t stop doing the code reviews and only do a patch description review. Nobody is suggesting that.
As I said, please include a concrete way in which the process description can be improved.
Note: Suggestion at the bottom, supporting information first.
At work, I use an approach where code review diffs are split into two (and a half) categories:
bugfix - the normal description for a bugfix outlines the issue and describes the solution (if it isn’t immediately obvious)
feature - this normally comes with a statement about the impact to the user, a screenshot or gif of it in action, and always links to a ‘ticket’ which contains all the relevant design and research information. Pulling in the design and research to the code review context might make it a bit overwhelming, but maybe that’s the only option
2.5. Architectural change. Architectural changes require an ADR (architectural decision record) which outlines the problem, considered solutions, proposed solution, basically what you’ve outlined in your proposal here. These work really well for large changes and I’d get worried if we stopped using them, but for day-to-day features like ‘add a filter here’ or ‘make the colours there more consistent’, the overhead of such documentation slows development down considerably.
I’d say it might be a good idea to differentiate between fixing papercuts and substantial changes to the system, just to avoid copy-and-paste, or tedious and irrelevant work on small changes.
Your list of “ingredients” is really similar to a RightClickSelect proposal’s requirements. I wonder if the devs could use RCS, too, as a launch point for proposals? That would streamline some things and keep it open to everyone.
I’m not gonna pretend all patch descriptions are perfect and there is no issue here, I can’t help wondering how much of an issue it really is? I interact quite a bit with new devs (i’m generally the first line in build support) and the main complaints i hear are based around people not knowing who to assign to their patch, nobody assigning someone for them when they don’t, and patches (assigned or not) just sitting there for days/weeks/months/years until the dev just gives up and vanishes.
Lets for a moment assume this is a bigger issue than I feel it is and it needs fixing, I am uncomfortable with a rigid set of rules for ‘fixing’ this, a one liner bugfix diff is different than a diff where we swap out our complete physics sim like mantaflow diff.
A patch by @sergey/@mont29 regarding threading imho is going to need a whole lot less explaining than a patch from a new dev wanting to make a change there, I expect the core devs to know what they doing, it is their area and I trust them to do the right thing, if i still think an alternative option is better, I can bring this up in code review and discuss it, there’s no reason for them to waste time on preemptively explaining why i’m wrong. From a new dev it would indeed be nice to know other options were considered but ultimately rejected since we cannot make the assumption strangers know what they are doing, some may, some may not.
So we either have to nail down when these rules apply and when they don’t or just accept they are pretty great guidelines and use them as such
I don’t agree here. Those are usually some non-trivial fixes which requires logic verification more than verifying code. Without knowing what the patch is actually solving doing verification beyond to simply typo fixes and such is not possible, and trying to guess what is it on a logic level the patch is addressing is not trivial from reading the code.
One related to this specific case thing. All those non-trivial decisions should be documented in code as a comment. This is because tracking down initial commit is not really trivial or impossible. Now, even if there is a detailed explanation in the code is still really helpful to have that in the patch description. The reason for this is because that explanation in the code wouldn’t usually be in the beginning in the patch, so you’d need to scroll and see what is going on before you can make sense of other cases. Having such information copy-pasted to a patch description allows to understand and verify changes done from the very beginning of the patch.
keyword here is ‘non trivial’ sure having a description in both code and diff/commit msg for a non trivial fix is great and should be encouraged, nobody is arguing against that.
however I don’t feel that adding a section in the D6189 description on why atomics were used over a mutex/spinlock/no locking at all would have been a super good use of mont29’s time.
I’m not against better descriptions on diffs (I go through great extent to justify my non trivial patches honestly, however if i fix a memleak, that extent is going to go waaaaaay down, it’s not my job to educate the world on how a memleak works in every single one I fix) I am just uncomfortable to blanket apply these new rules to every single diff, great guidelines, not so great rules
I don’t think anybody is talking about blindly applying those rules for every change. Use the common sense to see whether one point is applicable or not, as well as deciding how much into details it worth to go.
The way I see it is more of a checklist, which helps people to make sure presentation of their work is at a decent level. The goal here is to make code review more pleasant and not to be bureaucratic.
Here is an example of what I think matches my proposal: https://developer.blender.org/D6352. It’s not hard to write, but IMO gives a nice overview of the problem, the solution, alternatives and why I don’t choose them at this moment, shortcomings/limitations, and concrete points for the reviewers to discuss.
I always like to propose things for blender, and I usually work especially interface mockups, but it’s not something that programmers like to do. If any developer needs help for a concept of some feature to implement that tell me and I will help.
About the only thing I would add is that for anything that has a visual change I prefer to see both before and after captures to make it easier to compare. Not just a nice “after” image which forces me to track down that particular area in blender just to confirm how it looks now. That might seem a bit anal for otherwise really nice descriptions like in D6352 but it helps to make the proposal more self-contained.
IMO the big problem with the patch submission process is that the developers sometimes don’t see the patch or acknowledge that they have seen them even when you try contacting in other channels (blender.chat) you get no response.
It can be discouraging if you create a descriptive write up and you don’t know if anyone will look at your work.
Good design is a process and as such, I think the “think first, code later” approach has merit.
However, I feel it needs to be pointed out that no matter whether you’re an experienced hand or a fresh new face, the learning and exploration phase is about thinkering with different approaches and solutions to a problem (and may even lead to restating the problem several times).
It is only after one is well into the thinkering phase that one can begin to adopt a proper design mindset, which is to say that the design phases can be described as:
First, thinker and play around with different approaches to learn about the problem space
Second, begin to consider trade-offs inherent to the different approaches when taking into account the existing code base and design
Third, based on this knowledge try to describe the problem and the chosen solution without too many implementation details
Fourth, design and implement the proposed solution so that it fits into the larger context
Five, iterate per review
A developer who has experience with a given code base will have done lots of thinkering on it already, so their path through the “thinkering” and “consider trade-offs” phases is much quicker, because they will have already built up silent knowledge of the cul-de-sacs implicit in the project’s existing design.
In contrast, a new developer with no experience of the code base will need guidance to understand the inherent trade-offs and will need to follow the project design guidelines step-by-step before their proposals are of a sufficient standard.
Therefore, I think that what is really needed is a set of well-written design guidelines with examples of a good design process and its outcome in terms of a concrete improvement to Blender.
Once the new developer has seen and understood the outcome of these well-written design guidelines, the template you propose will make much more inherent sense and most developers will immediately understand which parts of it are useful in any given context.
In other words, I don’t think there’s a one-size-fits all solution to the problem of “good design”, but I do think that a solid project design guide will help generate consensus about which cases require going through the full blown design process (i.e. following the template) and which merely require a “Fix for broken foo” patch with a short description below the summary.
