Follow-up Comment #1, patch #4307 (project freeciv):

Hi Mark,

thank you for your patch.

> This is my first post
Welcome. Your next step is to address my feed back. If you think I'm wrong
about something post a comment that explains why. If you think I'm right about
something upload a fixed patch. More documentation can be found at:

I had a look at you patch. Some issues:
* city.h is from common, not server. Please put it in the correct group.
* Nit pick: You cover the case of subtracting as well. If you change the name
from _add to _change it becomes self documenting.
* You should inform the client when the city size changes. (Unless the client
belong to a player that can't see the city size change)
* Your patch makes a city of the size 0 possible. This is wrong. Destroying
the city properly when the size becomes 0 or simply refusing to set the size
to 0 are two possible ways to fix this.
* Please look for assertion errors when you test your code.

I managed to make you patch apply but it took some time. Advice on patch
creation to make it easier in the future:
* if this is accepted it will enter trunk, not 2.4. The patch should therefore
be against trunk. You can download trunk from
svn:// using your favorite SVN client. (If you
aren't familiar with SVN but know other version control systems plug ins
exists that let you use git, hg or bazaar as a SVN client)
* the patch should be for the Freeciv source root even if it only modifies
files in one sub folder. This way it won't be necessary to enter the sub
folder to apply it. Your SVN client will probably handle this for you (if you
use it to create the patch).
* You patch has line break issues. Hopefully your text editor allows you to
set line break style. Alternatively you can probably make your SVN client
handle it.


Reply to this item at:


  Message sent via/by Gna!

Freeciv-dev mailing list

Reply via email to