[Scummvm-devel] Thoughts on how to encourage incremental improvements

[Johannes Schickel]

Hi,

this has been bothering me about some time. So I want to give some basic 
guide, which I think helps in increasing our code quality.

First I want to give some general overview about committing code (to 
areas you do not maintain). This is in theory no problem at all. We 
still implement some basic rules for people who are new to ScummVM 
though. I will shortly recap these rules for newcomers, in a slightly 
adjusted fashion from what I have been told when I joined ScummVM as a 
developer:

  * You are free to work on your own engine/port.

    I think this does not need much explanation. If you joined the 
project because your port or engine was added, then you are the 
maintainer. So you are responsible for the code and thus have freedom to 
do as you please. You are limited by our portability, coding and 
formatting guides though.

  * If you want to have a new feature in the common code, then start a 
pull request/discussion about this.

    When you are new, you might not yet be familiar with our codebase 
and where things belong. So it makes sense to discuss your changes with 
the existing developers. If you are shy to discuss this in public on the 
first try, you are free to privately mail our core team.

  * If you notice a bug in the common code (and maybe have a bug fix), 
then start a pull request/discussion about this.

    We encourage you to report/fix bugs in our common code. But there is 
a possibility for controversal about how a bug is fixed or whether it's 
actually considered a defect. Discussing things with the maintainers of 
the code is always a benefit.
    - If your bug fix is fine, it is thankfully accepted.
    - If the maintainer thinks you are using the code wrong, there is 
the chance to update the documentation to make the purpose and usage of 
the code clearer. Also the maintainer can help you in changing your 
engine/port side code to work properly with the "intended" way the 
common code works.
    - If you actually extended the functionality it will help in 
creating a versatile solution by discussing your extension with the 
people who originally designed the  code.

  * If you get feedback on your code, try to see it as a benefit.

    This can be hard. Some people have the tendency to be rude, like 
myself, this is unfortunate and might get you angry. But everyone is a 
human after all. Try to accept their faults and believe they try to 
improve themselves and your code. Take this as a chance to learn from 
people who are in the project for some time already. This is also a 
chance to share your experience with other people by discussing their 
suggestions.

  * Do not be afraid to mail people or -devel when you have questions.

    Nobody bites (hard). Most of our developers are actually nice and 
try to be helpful when they have time. Take this as an opportunity to 
learn about the code and to save yourself some time/nerves.


Now I want to actually identify and explain four common problems, which 
can arise when you commit to code you do not maintain.

  (1) People are protective about their code.

    That says it all. Some people do not like changes from other people. 
Either because they do not trust them, they like to control "their" code 
or they think "their" code is just "better".

  (2) People disagree on what is a defect.

    Some people have different opinions of what is a bug. Sometimes they 
actually think "their" code was never meant to do what you think it 
should do. As a result some "misbehavior" you see might be just a result 
of the code never being designed to do what you want.

  (3) People disagree on how bugs should be fixed.

    Sometimes there is a disagreement on what is the actual cause for a bug.

  (4) People do not believe the feature you added is fine.

    This might be either because they do not think your new feature 
makes sense or because they disagree on how the feature is implemented.

All these problems might lead to conflicts. Since people hating each 
other is not healthy for individual people, our development team as a 
whole and our software, we need to prevent this from happening. Or at 
the very least try to make this as easy and peacefully as possible on 
everyone involved.

We need to consider the human factor in the problems I identified above. 
Everyone in our team has his/her own strengths and weaknesses. We come 
from different countries over the world, thus our customs might differ. 
What seems aggressive for one person might be normal tone for another 
person. Because of these reasons I propose a basic and at the same time 
sadly hard to follow mindset everyone should keep when interacting with 
other developers:

    Believe other (team) developers want to help in improving (your) 
code, your skills, (your) workflow, etc.

I know how hard this can be. Sometimes you have the feeling other people 
only criticize you, express your code is bad, are being rude, their 
suggestions are stupid, etc. In such cases it is a good idea to take a 
breath and think about it again. If you get the feeling the person you 
talk with is angry, try to believe he still wants the best and try to 
keep the situation calm. If you get the feeling you are angry, say sorry 
or wait till you calmed down again before you join the discussion. 
Otherwise all people involved might get frustrated and reduce their 
involvement.


Next I want to explain my views on the four problems mentioned above. I 
want to split this into two sides, the "maintainer" side, which I will 
cover first, and the "comitter" side. I will go through each point in 
the same order.

  (1) Being protective about your code is an understandable action, but 
we work on a project together. Everyone should be able to improve the 
code. I believe it is worth to work one oneself to get away from the 
idea that code you wrote is "yours". The code you contribute belongs to 
the whole team. Maintaing code means being responsible for it. If 
someone commits to code you maintain, try to talk with them when you 
feel strange about the change.

  (2) The distinction between what a code is supposed to do and what it 
is not is blurry. We try to write documentation to explain what the code 
guarantees you. This documentation might be unclear or even worse not 
exist. If people tell you the code you maintain does not work properly, 
take this as an opportunity to improve the code/documentation you 
maintain. Do not take this as an attack on yourself.

  (3) This is a serious problem. Some fixes people do are considered 
"workarounds" or simple "bug hiding" by other people. If someone commits 
a fix, he believes fine, and you disagree with try to talk with the 
developer. Ask him for his reasons. But as the very least do not take 
this as a blow into your face. Use his commit as a base to further 
improve the code you maintain.

  (4) This might even happen to you as a maintainer. If people want to 
discuss features take their feedback as a base for a fair discussion.

In general, if someone committed to the code you maintain try to improve 
it, when you do not like it.

Now onto the "committer" side.

  (1) If you get someone asking why you did the change, explain them 
why. If you think they are offensive, tell them about your feelings and 
try to stay calm. After all maintainers usually spent quite some time on 
the code and thus might have some insights or knowledge you do not have. 
Your contribution is still welcomed, since maintainers also have time 
constraints.

  (2) If you get feedback, there is really no way around to explain your 
reasons and accept that someone feels different about your changes. The 
goal is to improve the project. Try to take part in a productive 
discussion. And try to accept their opinion.

  (3) Someone thinks your bug fix is not a fix. This is a matter for 
discussion. It might be even that your change was a first step and 
people still come back at you, because they feel bad about the change 
because they are more familiar with the code. Do net let yourself be 
discouraged by people disagreeing on how bugs should be tackled.

  (4) You will have to live with this. (Big) features should generally 
be discussed on -devel or in a pull request first. So this hopefully 
will not happen because of commits to our central repo. Still try to 
take this opportunity to explain your reasons and improve the feature('s 
implementation). If the agreement comes to your feature being removed 
accept it.


On a more general side I want to propose some workflow when you find 
issues with code other people maintain:

  * When you find a bug and you have a fix you really believe fine and 
you are really sure about it, you are free to commit it. Always. To any 
code. Also try to ping the maintainer for feedback/notice never the 
less. A special comment on github's commit log directed to the 
maintainer is helpful here. Another way is to ping the developer via 
mail. If you do not know who maintains the code, ask our core team or 
drop a public note on github's commit log.

  * In case you are unsure about your fix, please try to contact the 
maintainer beforehand. If you are not aware of any maintainer go with a 
pull request. If there are no replies in the first hours or days after 
you did this do not be discouraged, people have a real life too.

  * In case you think the code does not make any sense, talk to the 
maintainer. If there is no maintainer or you do not know him ping -devel 
for thoughts.

  * If there is a comment on your changes behave like I tried to outline 
previously.

In general I think it is nice to talk a maintainer first, when you never 
touched the part of the code he is reponsible for. I think pinging 
people, even after you committed changes, will help in pointing them to 
issues and thus on improving them.

If you are a maintainer you should try to behave as follows:

  * When you see a commit you feel strange about, talk to the commiter 
first. Be nice to the committer too!

  * When you get an query about code you maintain try to answer at your 
best knowledge. If you temporarily do not have time please write a short 
reply that you will answer later. If possible also point them to other 
people they can discuss with.


I think that is basically sums up my feelings about this topic. If you 
want to discuss this feel free to.

// Johannes