I would like to start a thread on understanding how can we speed up the process of merging Pull Requests to openmrs core without compromising on quality. It will be good if we can listen from various core committers on to what they usually check in a PR. Listed down some basic things that can be done to save the reviewer’s time as well as the developer’s. Especially if the developers are not based out of EST timezone.
Good quality test cases covering possible scenarios.
Design reviews, if the PR is a feature (for e.g. Order Sets, Episode of Care). I hope we do this in design forums. Do we need to schedule this in advance or can it be adhoc?
Code reviews, if the PR is a big feature. I hope Developer forums are the right place for these. Do we need to schedule this in advance or can it be adhoc?
Great topic, and especially timely with the back and forth over the Order Sets pull request!
Another set of things to look out for is in the Pull Request Tips wiki page.
To the @dev5 s who have been doing code reviews: please take some time to make sure that the Coding Conventions wiki page is up to date. (For example I don’t see anything there about @since.)
@bharatak, I think the Order Sets PR example raises a couple issues:
some design points that Burke, Wyclif, and I had our heads weren’t communicated effectively to you all before work started
some conventions aren’t explicitly written anywhere (e.g. sort_weight should be double, not int)
it’s hard to review a very large chunk of work in one go (realistically, a reviewer will have to take several passes at it, because you can’t effectively review it all in one go)
Some of this is inevitable, but I think the biggest thing we should have done differently is had more regular short communications while the code was being written. Basically, any large chunk of ongoing work that anyone hopes to have merged to openmrs-core quickly should have a dedicated thread in OpenMRS Talk, with an update for every few days of work. (I would particularly structure the communication around English-language descriptions of the design and approach, e.g. as new or refined “Acceptance Criteria” or expected behaviors.)
In addition to this we’re generally always happy to spend more time on design calls designing big features that people are working on (or about to work on), but it’s worth pointing out that these two topics (Order Sets and Episodes of Care) have not really lacked for design call discussions, so maybe what we really need to work on is transitioning from the design discussion to ready-for-dev work.
Thanks for bringing this up, i agree that we can do better to make this process faster and i also agree with most of what Darius had to say.
The key thing personally I see here is the timezone issue, my assumption is that pull requests from devs in time zones in the east should be of higher priority for devs from time zones with less time differences than devs in the west but the time zone of a dev who issued a pull request isn’t obviously known by just looking at the pull request, but in the case of the OrderSets pull request we actually do know because we know where most of their team members are located, so i think we could’ve had Daniel or Rafa helping with reviewing that code.
We need to have more developers reviewing and merging pull request in the future as Burke normally suggests instead of most of the time being Rafa, Daniel, Darius and myself, Lluis also helps with reviews which is great, wish we could have more Lluis’ , with this we do have a higher shot at having more reviewers from multiple time zones performing timely reviews.
The primary reason we’ve been elevating the privileges of other devs e.g. from Bahmni and in general in the community is because we’re hoping to get more devs involved in the reviewing and merging process. Ideally merging the current OrderSets pull request should actually be done by the team at Bahmni, it is forward porting it to master and 2.0.x that will require the core devs to do an extra check
, with this we do have a higher shot at having more reviewers from multiple time zones performing timely reviews.