[GNOME] Providing feedback on patches and branches

awesome advert:

Did you know that GXml has a mailing list now?  gxml-list at gnome dot org!

  

Finding time to review

 

A week before GUADEC, the spectacular Daniel Espinosa let me know of work he's doing on GXml serialization in a new branch.  Sadly, it's taken me three weeks to provide adequate feedback.



There were three obstacles to this. 

1. having my HP tc4400 laptop (her name was clarity, after Claire Danes) die shortly thereafter.  


2. going to GUADEC (there was a lot of new work to do while there!)


3. my thesis (sadly, I don't have my summer totally free)


4. other GSOC goals

I feel like such a long delay, even if the causes seem reasonable, is terrible for encouraging good contribution's like Daniel.  I'm lucky Daniel is currently  motivated and has goals.



Doing the review



Because the new work is on a branch, and the changes are a bit extensive, it was a bit challenging keeping track of all the changes.  That's opposed to a set of smaller patches or contained changes, which I might be able to analyse in parts.  Because of cross-file changes and some code-reorganisation, I ended up using emacs ediff and some hand-editing to ease my comparison.  Sometimes changes look bigger on the outside, until you realise that fundamentally a lot of the new logic is the same.



I finally ended up spending about 5 hours reviewing it, which feels a little excessive, and I hope I get better at it.  As a graduate teaching assistant at my university, I'm used to reviewing students' code (which is more predictable and less complex).  I think one problem is that I didn't want to miss anything; I feel as though a quicker and sloppier review might actually be preferable for its quicker return time, than having a thorough and careful one that takes forever to find the time to start.



Providing feedback



One of the hardest parts is providing meaningful and friendly resistance to changes.  I want to make sure that changes are safe (smaller and more precise ones are preferable to minimise new bugs) and necessary (changing APIs without a clear benefit is painful for existing developers), but I don't want to overly discourage a submitter.



I tried to ask specific questions about the motivations behind certain changes, and tried to propose smaller changes that could accomplish the same purpose.



Hopefully I will prove more responsive in the future and Daniel's work will improve GXml's serialization support. :D



Advice from you



I'd appreciate any advice you have for reviewing code and encouraging submitters.  I heard that cursing at contributors works well for some well-known maintainers, but I don't think GXml is quite popular enough to afford that level of abuse. :)