View unanswered posts | View active topics It is currently Tue Sep 25, 2018 7:46 am



Reply to topic  [ 9 posts ] 
Code Reviewers and Review Requests 
Author Message
Moderator
User avatar

Joined: Tue Jan 15, 2008 2:31 pm
Posts: 15181
Location: Home sweet home!
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!


Sat May 25, 2013 4:07 pm
Profile WWW
Moderator
User avatar

Joined: Tue Jan 15, 2008 2:31 pm
Posts: 15181
Location: Home sweet home!
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!


Sat May 25, 2013 4:07 pm
Profile WWW
LQFP144 - On Top Of The Game
User avatar

Joined: Mon Mar 31, 2008 12:47 am
Posts: 568
Location: New Zealand, land of the long white burnout
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


Sun May 26, 2013 9:28 am
Profile WWW
Moderator
User avatar

Joined: Tue Jan 15, 2008 2:31 pm
Posts: 15181
Location: Home sweet home!
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!


Sun May 26, 2013 11:13 am
Profile WWW
LQFP112 - Up with the play

Joined: Sat Apr 24, 2010 1:42 am
Posts: 119
Location: New Zealand
Is there a clean way to 'sign off' a commit as being reviewed?


Mon May 27, 2013 1:32 am
Profile
LQFP144 - On Top Of The Game
User avatar

Joined: Mon Mar 31, 2008 12:47 am
Posts: 568
Location: New Zealand, land of the long white burnout
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


Mon May 27, 2013 1:59 am
Profile WWW
Moderator
User avatar

Joined: Tue Jan 15, 2008 2:31 pm
Posts: 15181
Location: Home sweet home!
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!


Mon May 27, 2013 2:36 am
Profile WWW
LQFP144 - On Top Of The Game
User avatar

Joined: Mon Mar 31, 2008 12:47 am
Posts: 568
Location: New Zealand, land of the long white burnout
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


Mon May 27, 2013 5:55 am
Profile WWW
LQFP112 - Up with the play
User avatar

Joined: Tue Jan 17, 2012 11:21 am
Posts: 171
Location: Ponte de Lima, Portugal
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)


Mon May 27, 2013 5:56 pm
Profile
Display posts from previous:  Sort by  
Reply to topic   [ 9 posts ] 

Who is online

Users browsing this forum: No registered users and 2 guests


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Search for:
Jump to:  
Powered by phpBB® Forum Software © phpBB Group
Designed by ST Software for PTF. ColorizeIt.