Code Reviewers and Review Requests

FreeEMS topics that aren't specific to hardware development or firmware development.
Post Reply
User avatar
Fred
Moderator
Posts: 15431
Joined: Tue Jan 15, 2008 2:31 pm
Location: Home sweet home!
Contact:

Code Reviewers and Review Requests

Post by Fred »

This post is a list (that I intend to maintain) of people who I'd like to see reviewing code on a regular basis, and what they're suited to reviewing:
  • Me - firmware and OLV (I could do EMStudio too, but don't have enough time)
  • Sean0 - firmware and EMStudio
  • MrOnion - firmware, OLV and EMStudio
  • johntramp - firmware and EMStudio
  • malcom2073 - firmware and EMStudio
  • MerpCore - firmware and EMStudio
  • BenFenner - OLV
To help ensure high quality in every project each commit should be reviewed. It doesn't take long to do (unless the commit is huge or complex), so it's very worth while to avoid mistakes slipping in.

This is not only really good for the quality of the project, it's also a solid way to become familiar with the code bases and that's gotta be good for both you and the project.

A guide on how to do a basic review is available here: viewtopic.php?f=41&t=1683

Please post up links/issues/commit hashes of stuff that you actively want reviewed!

Fred.
DIYEFI.org - where Open Source means Open Source, and Free means Freedom
FreeEMS.org - the open source engine management system
FreeEMS dev diary and its comments thread and my turbo truck!
n00bs, do NOT PM or email tech questions! Use the forum!
The ever growing list of FreeEMS success stories!
User avatar
Fred
Moderator
Posts: 15431
Joined: Tue Jan 15, 2008 2:31 pm
Location: Home sweet home!
Contact:

Re: Code Reviewers and Review Requests

Post by Fred »

Right now I'd like to see 3 commit-sets of mine reviewed and three associated issues closed. Two are a pair currently assigned to MrOnion, and the other one to Sean0.

http://issues.freeems.org/view.php?id=243 seank-efi
http://issues.freeems.org/view.php?id=213 & http://issues.freeems.org/view.php?id=844 MrOnion

Others can dive in and grab these any time, they're not owned by the person they're assigned to (in this case).

I'd like to see any commit of Mike's that makes it into dev/master branches reviewed too. I hate regressions, let's help him eliminate those with good process.

Fred.
DIYEFI.org - where Open Source means Open Source, and Free means Freedom
FreeEMS.org - the open source engine management system
FreeEMS dev diary and its comments thread and my turbo truck!
n00bs, do NOT PM or email tech questions! Use the forum!
The ever growing list of FreeEMS success stories!
User avatar
sry_not4sale
LQFP144 - On Top Of The Game
Posts: 568
Joined: Mon Mar 31, 2008 12:47 am
Location: New Zealand, land of the long white burnout
Contact:

Re: Code Reviewers and Review Requests

Post by sry_not4sale »

Are you using the github "pull request" inteface for code review?
Owner / Builder: 1983 Mazda Cosmo 12at (1200cc 2-rotor turbo) coupe [SPASTK]
165hp @ 6psi standard - fastest production car in japan Oct 82
User avatar
Fred
Moderator
Posts: 15431
Joined: Tue Jan 15, 2008 2:31 pm
Location: Home sweet home!
Contact:

Re: Code Reviewers and Review Requests

Post by Fred »

No, but that's not a bad idea, I guess. I'm considering setting up Gerrit, but it's fairly rigid in the way it works. For now the volume is low enough that the issue tracker is enough. The hash goes in there anyway.
DIYEFI.org - where Open Source means Open Source, and Free means Freedom
FreeEMS.org - the open source engine management system
FreeEMS dev diary and its comments thread and my turbo truck!
n00bs, do NOT PM or email tech questions! Use the forum!
The ever growing list of FreeEMS success stories!
johntramp
LQFP112 - Up with the play
Posts: 119
Joined: Sat Apr 24, 2010 1:42 am
Location: New Zealand

Re: Code Reviewers and Review Requests

Post by johntramp »

Is there a clean way to 'sign off' a commit as being reviewed?
User avatar
sry_not4sale
LQFP144 - On Top Of The Game
Posts: 568
Joined: Mon Mar 31, 2008 12:47 am
Location: New Zealand, land of the long white burnout
Contact:

Re: Code Reviewers and Review Requests

Post by sry_not4sale »

On github you just click "merge", or comment +1
Owner / Builder: 1983 Mazda Cosmo 12at (1200cc 2-rotor turbo) coupe [SPASTK]
165hp @ 6psi standard - fastest production car in japan Oct 82
User avatar
Fred
Moderator
Posts: 15431
Joined: Tue Jan 15, 2008 2:31 pm
Location: Home sweet home!
Contact:

Re: Code Reviewers and Review Requests

Post by Fred »

I'll probably require commits to have a signed off by field in them in future. Gerrit takes care of the reviewed/not thing nicely, but as mentioned, it's pretty serious/heavy infrastructure.
DIYEFI.org - where Open Source means Open Source, and Free means Freedom
FreeEMS.org - the open source engine management system
FreeEMS dev diary and its comments thread and my turbo truck!
n00bs, do NOT PM or email tech questions! Use the forum!
The ever growing list of FreeEMS success stories!
User avatar
sry_not4sale
LQFP144 - On Top Of The Game
Posts: 568
Joined: Mon Mar 31, 2008 12:47 am
Location: New Zealand, land of the long white burnout
Contact:

Re: Code Reviewers and Review Requests

Post by sry_not4sale »

Fred wrote:I'll probably require commits to have a signed off by field in them in future. Gerrit takes care of the reviewed/not thing nicely, but as mentioned, it's pretty serious/heavy infrastructure.
Overkill for this project right now IMHO. Especially since you use github. I've setup a few Gerrit's now, but only because we needed our own infrastructure. Also, Gerrit is hard on servers!
Owner / Builder: 1983 Mazda Cosmo 12at (1200cc 2-rotor turbo) coupe [SPASTK]
165hp @ 6psi standard - fastest production car in japan Oct 82
User avatar
masterkorp
LQFP112 - Up with the play
Posts: 171
Joined: Tue Jan 17, 2012 11:21 am
Location: Ponte de Lima, Portugal

Re: Code Reviewers and Review Requests

Post by masterkorp »

sry_not4sale wrote: Overkill for this project right now IMHO. Especially since you use github. I've setup a few Gerrit's now, but only because we needed our own infrastructure. Also, Gerrit is hard on servers!
Could not agree more.
Git am, has a sign option, you can sign merges too (In no-ff mode)
Post Reply