widelands-dev team mailing list archive

[Hans Joachim Desserud <launchpad@xxxxxxxxxxxx>]

Hi, just a post-review comment:

Please write more descriptive commit messages when merging to trunk. This makes it easier to tell what was changed, both when reading through the commit log, but also when someone wants to know why a block of code was added six months later. 

Including the bug number is great, but it should say a bit more about how the code has changed from the previous commit (and depending on the situation something about the issue which was fixed.) The description you have at the beginning of this merge proposal is a good starting point.

As a hypothetical example; if you're looking up the history/changes of a piece of code you're working on; "Fixed bug 123" doesn't say a lot unless you remember all the bug numbers. You can of course look it up in a browser and skim through it, but then the next code line was added to fix another bug.

If it instead said "Ports now check if it has a ship available before starting expeditions. Used to attempt to load wares onto nothing and crashed. (Fixes bug 123)" it immediately gives you a context on why the code was added and what problem it solves. And when moving on to the next line, you can see that it was added to "Fixed warning. Doesn't need to assign value to unused variable".

Ideally, the commit log should tell a story about how the code has evolved over time.

Other than that, keep up the good work. :)
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1428396/+merge/256857
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1428396.