[Announcement] Code review process

Development directions, tasks, and features being actively implemented or pursued by the development team.
Post Reply
klauss
Elite
Elite
Posts: 7243
Joined: Mon Apr 18, 2005 2:40 pm
Location: LS87, Buenos Aires, República Argentina

[Announcement] Code review process

Post by klauss »

We have a freshly set up code review tool at our disposal.

You can access it here.

Instructions on how to use it here.

All patches should be sent to be reviewed, unless you're a VS dev with direct SVN access and the patch is trivial. Even for us devs with direct SVN access, it is recommended we submit the patches for review before committing them (or after), to increase the chances of quickly detecting any problems with new code (and possibly old, related code).

Data changes may not work very well with the tool, so I'll leave it up to the submitter to decide how to review data-side changes. If the changes relate to python scripts or xml files, it should work fine, and submitting them to review should be beneficial. But if it involves images or binary data I'm not so sure.

For units.csv changes, any reviews should be informal (post a patch in the patch tracker, explain what the changes are) and post-commit (commit the changes as soon as they're tested and working), to avoid conflicts and because usualy code review tools don't work well with units.csv.

When creating a new topic for review, set the following fields, besides the usual title and description:
  • Repository: must be vegastrike SVN. CVS isn't used anymore.
  • Project: pick an appropriate project, depending on what your patch is about.
  • Reviewers: minimally: klaussfreire, pheonixstorm. You can add others too.
  • Cc: Should we create a mailing list perhaps?
  • State: Leave Open
For reviewers, a few terms should be used to convey approval or disapproval:
  • ship it or LGTM (looks good to me), to approve the changes
  • don't commit to reject them - ie, if you find a game-breaking bug
  • PTAL (please take another look), if you made changes or if you're requesting a re-review of your code
Last edited by klauss on Sun Mar 06, 2011 9:04 pm, edited 1 time in total.
Reason: Expanded the procedure to include reviewer replies and units.csv-specific procedures
Oíd mortales, el grito sagrado...
Call me "Menes, lord of Cats"
Wing Commander Universe
charlieg
Elite Mercenary
Elite Mercenary
Posts: 1329
Joined: Thu Mar 27, 2003 11:51 pm
Location: Manchester, UK
Contact:

Re: Code review process

Post by charlieg »

This should be made sticky.
Free Gamer - free software games compendium and commentary!
FreeGameDev forum - open source game development community
pheonixstorm
Elite
Elite
Posts: 1567
Joined: Tue Jan 26, 2010 2:03 am

Re: Code review process

Post by pheonixstorm »

:?: :?: :?: um eh? sticky an announcement??
Because of YOU Arbiter, MY kids? can't get enough gas. OR NIPPLE! How does that mkae you feeeel? ~ Halo
charlieg
Elite Mercenary
Elite Mercenary
Posts: 1329
Joined: Thu Mar 27, 2003 11:51 pm
Location: Manchester, UK
Contact:

Re: Code review process

Post by charlieg »

Oh, oops. Sorry, there's an ambiguity in the boards. I always check the forum via the newposts feature and it's really not-obvious what is sticky and what is an announcement (versus standard posts). The only difference is the icon, and the difference between icons is pretty subtle. (I would have expected an [announcement] preceeding tag.)
Free Gamer - free software games compendium and commentary!
FreeGameDev forum - open source game development community
breese
Bounty Hunter
Bounty Hunter
Posts: 152
Joined: Thu Sep 02, 2010 8:00 pm

Re: [Announcement] Code review process

Post by breese »

klauss wrote:We have a freshly set up code review tool at our disposal.
Bad news. You cannot create a new review unless you have admin access.
klauss
Elite
Elite
Posts: 7243
Joined: Mon Apr 18, 2005 2:40 pm
Location: LS87, Buenos Aires, República Argentina

Re: [Announcement] Code review process

Post by klauss »

That makes no sense...
Oíd mortales, el grito sagrado...
Call me "Menes, lord of Cats"
Wing Commander Universe
pheonixstorm
Elite
Elite
Posts: 1567
Joined: Tue Jan 26, 2010 2:03 am

Re: [Announcement] Code review process

Post by pheonixstorm »

I'm still looking at a solution to this problem. So far i'm stuck on the following for bug/feature/review

* Flyspray
* Thebuggenie
* MantisBT
* Buggzilla (far too much for a project this size I think)
* ReviewBoard (what little ive seen I like this suggestion klauss, thanks for the tip!)
* Traq (Trac clone in PHP)
Because of YOU Arbiter, MY kids? can't get enough gas. OR NIPPLE! How does that mkae you feeeel? ~ Halo
klauss
Elite
Elite
Posts: 7243
Joined: Mon Apr 18, 2005 2:40 pm
Location: LS87, Buenos Aires, República Argentina

Re: [Announcement] Code review process

Post by klauss »

I've seen no indication on CodeStriker's manual that you need any kind of administrative access to create "reviews", which is not the same as project.

Does this link not work?

(if it does, I'll add it to the initial post)
Oíd mortales, el grito sagrado...
Call me "Menes, lord of Cats"
Wing Commander Universe
pheonixstorm
Elite
Elite
Posts: 1567
Joined: Tue Jan 26, 2010 2:03 am

Re: [Announcement] Code review process

Post by pheonixstorm »


Visit project vegastrike
Pheonix Rising
Account
Log out

This function requires admin access.

© 2011 Geeknet, Inc. Terms of Use - Privacy Policy
Because of YOU Arbiter, MY kids? can't get enough gas. OR NIPPLE! How does that mkae you feeeel? ~ Halo
klauss
Elite
Elite
Posts: 7243
Joined: Mon Apr 18, 2005 2:40 pm
Location: LS87, Buenos Aires, República Argentina

Re: [Announcement] Code review process

Post by klauss »

Ok, that sucks.

I just posted a request for enhancement to source forge. In the meanwhile, I guess I'll have to ask for anyone interested in posting for review to post here (and anyone with administrative access can flag that patch as taken care of on the board).

I'll post whenever the situation is fixed. I'll pester SF support staff until it is :)
Oíd mortales, el grito sagrado...
Call me "Menes, lord of Cats"
Wing Commander Universe
pheonixstorm
Elite
Elite
Posts: 1567
Joined: Tue Jan 26, 2010 2:03 am

Re: [Announcement] Code review process

Post by pheonixstorm »

(and anyone with administrative access can flag that patch as taken care of on the board).
And this is why we need some new admins... :lol:
Because of YOU Arbiter, MY kids? can't get enough gas. OR NIPPLE! How does that mkae you feeeel? ~ Halo
Deus Siddis
Elite
Elite
Posts: 1363
Joined: Sat Aug 04, 2007 3:42 pm

Re: [Announcement] Code review process

Post by Deus Siddis »

klauss wrote: In the meanwhile, I guess I'll have to ask for anyone interested in posting for review to post here (and anyone with administrative access can flag that patch as taken care of on the board).
What is the process for submitting balance changes to uncompiled files like units.csv or an altered python script?

Also I have a couple newbe questions. If several people are making changes to different parts of the same file (say units.csv :)) at the same time, subversion can easily merge those changes together right? Assuming yes, what is the best way to submit such files for review with versioning intact?
klauss
Elite
Elite
Posts: 7243
Joined: Mon Apr 18, 2005 2:40 pm
Location: LS87, Buenos Aires, República Argentina

Re: [Announcement] Code review process

Post by klauss »

Usually SVN can merge most changes happening on different aspects of the game (ie, different functions, different units, etc).

Units.csv is a different beast, as most editors will introduce bogus changes al over the place when importing/exporting/importing/exporting.

For the rest, xml files, python scripts, code review should work just dandy.

In any case, the procedure is, if you can post a review as suggested in the initial post, to do just that. If not, because SF has a configuration issue that only allows admins to post last time I checked (I know, ridiculous), then post the patch in the patches section of the issue tracker.
Oíd mortales, el grito sagrado...
Call me "Menes, lord of Cats"
Wing Commander Universe
Deus Siddis
Elite
Elite
Posts: 1363
Joined: Sat Aug 04, 2007 3:42 pm

Re: [Announcement] Code review process

Post by Deus Siddis »

klauss wrote: Units.csv is a different beast, as most editors will introduce bogus changes al over the place when importing/exporting/importing/exporting.
So what's the procedure for changes to units.csv?

I have an overhauling balance proposal that affects the scale, thrust, torque and governor settings for almost all ships, stations and turrets; all changes to units.csv. But because changes to this file are so common, while the proposal is being reviewed numerous changes will have been applied to it's counterpart self in svn. So by the time the patch is approved it can no longer be applied.
then post the patch in the patches section of the issue tracker.
And a patch is just the modified file without any subversion data attached, right?
pyramid
Expert Mercenary
Expert Mercenary
Posts: 988
Joined: Thu Jun 15, 2006 1:02 am
Location: Somewhere in the vastness of space
Contact:

Re: [Announcement] Code review process

Post by pyramid »

For units.csv especially, it is impossible to see the change as the full line is marked. The line is however so long that one cannot easily spot the change anyway.

In my opinion it doesn't make sense to introduce too much bureaucracy to the change process. It does discourage contributors.

Some code changes should be reviewed though, but not all of them. I wouldn't want to go through a review process whenever I remove some unused variables. It is still a useful process, though I am not sure how to find a good balance between advancement and prevention of bugs due to impact on other functionality, for this should be the main purpose of code review. Whenever coding, we must remember that the engine is used by various mods, and their functionality might be impacted leading to undesired behavior.

As for VS data, I'd go easy with that and without code review.
pheonixstorm
Elite
Elite
Posts: 1567
Joined: Tue Jan 26, 2010 2:03 am

Re: [Announcement] Code review process

Post by pheonixstorm »

Deus Siddis wrote: So what's the procedure for changes to units.csv?

I have an overhauling balance proposal that affects the scale, thrust, torque and governor settings for almost all ships, stations and turrets; all changes to units.csv. But because changes to this file are so common, while the proposal is being reviewed numerous changes will have been applied to it's counterpart self in svn. So by the time the patch is approved it can no longer be applied.

And a patch is just the modified file without any subversion data attached, right?
If its the same one you sent me its going to have a negative impact on game play. I have been trying to test it as often as I can but kids keep sucking up my time..
Because of YOU Arbiter, MY kids? can't get enough gas. OR NIPPLE! How does that mkae you feeeel? ~ Halo
Deus Siddis
Elite
Elite
Posts: 1363
Joined: Sat Aug 04, 2007 3:42 pm

Re: [Announcement] Code review process

Post by Deus Siddis »

pheonixstorm wrote: If its the same one you sent me its going to have a negative impact on game play. I have been trying to test it as often as I can but kids keep sucking up my time..
Yeah it is the same one. Could you give me the specific issues you've noticed with it so far?

I'm actually already aware of the issues with slowing down travel; SPEC would need a speed increase to compensate and the autopilot needs to not shut itself off prematurely (that is not before SPEC no longer offers any advantage). Also better control of heavily cargo laden vessels will need further rebalancing.

But any other possible issues, I need more feedback for. That's why I'm asking if the code review process is the place to find it.
safemode
Developer
Developer
Posts: 2150
Joined: Mon Apr 23, 2007 1:17 am
Location: Pennsylvania
Contact:

Re: [Announcement] Code review process

Post by safemode »

you could just make it a rule that if your patch changes things across subsystems then you get your patch reviewed first. Since VS is not very compartmentalized yet, you could generalize that to if your patch is big it needs to be reviewed.

I prefer to work in a sub-branch and sync back to trunk because i like the idea of a trunk that only gets vetted commits. Most changesets would probably rapidly be applied to both the sub-branch and the trunk but at least that sub-branch is in sync and the dev is familiar with how to handle syncing to trunk so when large projects do come up, they are competent in working on their own temporarily isolated branch and are eventually able to sync back huge changes to trunk without screwing everything up.
Ed Sweetman endorses this message.
Deus Siddis
Elite
Elite
Posts: 1363
Joined: Sat Aug 04, 2007 3:42 pm

Re: [Announcement] Code review process

Post by Deus Siddis »

safemode wrote: you could just make it a rule that if your patch changes things across subsystems then you get your patch reviewed first.
This affects only units.csv (though it may be supported by further changes to dynamic_mission.py.)
I prefer to work in a sub-branch and sync back to trunk because i like the idea of a trunk that only gets vetted commits.
That might be preferable here too, as long as enough people tested the branch and returned feedback, the more detailed the better. But from what Klauss said, it is sounding like that system isn't easily possible with units.csv. Which is important because most all gameplay and graphical changes go through that one file.
klauss
Elite
Elite
Posts: 7243
Joined: Mon Apr 18, 2005 2:40 pm
Location: LS87, Buenos Aires, República Argentina

Re: [Announcement] Code review process

Post by klauss »

Deus Siddis wrote:
then post the patch in the patches section of the issue tracker.
And a patch is just the modified file without any subversion data attached, right?
A patch is a diff, svn diff, or with Tortoise the "create patch" command.

But, as stated, it doesn't go well with units.csv. For units.csv, I'd explain your changes in text, and apply an informal quick review - perhaps even post-commit.

For trivial changes, as pyramid says, there should be no review - but be careful with what you consider trivial changes. When in doubt, even a post-commit review is better than no review.
Oíd mortales, el grito sagrado...
Call me "Menes, lord of Cats"
Wing Commander Universe
klauss
Elite
Elite
Posts: 7243
Joined: Mon Apr 18, 2005 2:40 pm
Location: LS87, Buenos Aires, República Argentina

Re: [Announcement] Code review process

Post by klauss »

Deus Siddis wrote:That might be preferable here too, as long as enough people tested the branch and returned feedback, the more detailed the better. But from what Klauss said, it is sounding like that system isn't easily possible with units.csv. Which is important because most all gameplay and graphical changes go through that one file.
We could make it possible for units.csv - if you divide the columns in various files, we could make the code fetch them from various files, and have units_stats, units_graphics, units_whatever.csv (just examples there), so people could work in units_stats while others work in units_whatever.

But we'd have to:
  • Figure out which columns to slice and how
  • Implement it in the engine (not really hard)
But, atm, I don't think branches are good for units.csv changes.
Oíd mortales, el grito sagrado...
Call me "Menes, lord of Cats"
Wing Commander Universe
Deus Siddis
Elite
Elite
Posts: 1363
Joined: Sat Aug 04, 2007 3:42 pm

Re: [Announcement] Code review process

Post by Deus Siddis »

klauss wrote: A patch is a diff, svn diff, or with Tortoise the "create patch" command.

But, as stated, it doesn't go well with units.csv. For units.csv, I'd explain your changes in text, and apply an informal quick review - perhaps even post-commit.
Okay thanks, I'll try to use these methods for their respective file types (csv versus xml and py).
Post Reply