Reviewing PR's

[nicholasjhenry]

Some ideas I had today while working with PR's, will transfer them to a checklist:

• more than just reviewing a diff on Github
• review the original issue that the PR is going to fix
• download the code and do some exploratory testing
• link the PR with the original issue (via a comment) unless the commit message includes a reference to the original issue
• review the original issue that the PR is related to, does it close that issue, or are there some outstanding tasks, create those tasks
• does the PR comply with contributing guidelines?

[benichu]

Good stuff, that also make me think about the step before that:

1. how do we know what is to be reviewed, for example: If I look at the pull requests list, how do I immediately know which one I can take care of and if everything is fine with it, merge.
2. how do we enforce having a main code-reviewer on the PR

And also, before merging, I think we should ask people to squash their commits (ex: Rails: http://edgeguides.rubyonrails.org/contributing_to_ruby_on_rails.html#issue-a-pull-request 5.17.1)

[cawel]

Good stuff indeed. I think there should also be some guidelines for code reviews (which may or may not be a different thing than what is done before merging into master). My thoughts in bulk, touching on less technical aspects:

• There should be a trace in the issue about who reviewed the code.
• It should not be a problem if code is reviewed by more than one person. In fact, it should be encouraged. An community-driven open-source project like Montreal.rb is meant to have an educational aspect to it. I think code reviews fit perfectly in that perspective.
• It should be emphasized that code reviews should be done by peers regardless of their experience.

This project is not some product made within some entrepreneurial context, where productivity is often a primary factor. Rather, I think this project's codebase should ultimately be revered as an example of clean and maintainable code – and code reviews are instrumental in meeting that objective – made by cool people :sunglasses: Nothing less :)

[benichu]

I totally agree with @cawel about the educational aspect of code-reviews and encouraging any level of code-reviewers to participate. That's also why I am definitely not trying to enforce what I think is a very good git workflow in a professional context (briefly described here: https://github.com/montrealrb/Montreal.rb/pull/137#issuecomment-162263924)

We just need to find the right balance between keeping the project realistically manageable by the maintainer's, easy to contribute and discuss by the contributors but at the same time by making sure that pull requests do not stay open too long because we haven't made the merging process/responsibility clear enough.

[karimmtarek]

I know this might be irrelevant to this discussion, but I think using sometging like https://reviewable.io/ or similar code review services would make the code review process a bit more manageable.