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: * http://freeciv.wikia.com/wiki/Testing_Tips * http://freeciv.wikia.com/wiki/Coding_Style * http://freeciv.wikia.com/wiki/How_to_Contribute 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://svn.gna.org/svn/freeciv/trunk 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: <http://gna.org/patch/?4307> _______________________________________________ Message sent via/by Gna! http://gna.org/ _______________________________________________ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
