A recent discussion on GitHub (link) triggered a discussion about the protocol to use for work that takes longer to complete and/or is experimental.
So far, it seems the team reached the following consenus:
- Pull request with a title prefixed with
[WIP]are not done and ready for proper review. Early feedback is welcome though. - Pull requests with a title prefixed with
[DO NOT REVIEW]should not be looked at at all.
While the convention proposed above may work for us, who work on LoopBack full-time, I am pretty sure it won't be so obvious to the maintainers from our community and casual visitors to our project. I am arguing that [WIP] prefix is not good enough for us either - how should the reviewers know which parts of the patch are in progress and which are ready for review?
The pull request description should be explicit about the following:
The purpose of the pull request.
Is this an experimental work that will be thrown away later in favour of more rigorous implementation?
Is it a long-term work with a slow pace, or something that we want to finish soon, typically as part of our current milestone?
What kind of feedback is wanted
Which part of the proposed changes are ready for review? What should be ignored by reviewers for now?
- high-level design only vs. detailed review of code
- individual files/packages/areas to review/ignore
- tsdoc comments
- test coverage (or lack of)
- etc.
In general, the pull request author should explicity write what kind of feedback they are looking for.
This will save everybody's time, because we can avoid comments and discussions about topics that are not relevant yet.
As the popular joke goes:
Code reviews:
10 lines of code = 10 issues.
500 lines of code = "looks fine."
Reviewing large pull reqests is not fun. Either we take the review process too lightly and risk introducing problems that will bite us in the future, or else we spend a lot of time reviewing the code, applying changes asked for, making even more comments and changes, wasting a lot of time on unnecessary rework.
The situation is even worse when the pull request is affecting the framework design in a way that was not discussed before. The review of such pull request becomes a painful mixture of high-level design discussion with low-level code reviews.
I'd like us to get into the habit of splitting any major development into two steps:
Explore the problem & solution space. Focus on high-level design, non-trivial edge cases, etc.
Focus on documenting as much as you can:
- What are the different use cases that defined the constraints for possible solutions?
- What options you have rejected after consideration and why?
- What are the pros and cons of the proposed solution you arrive at?
- What limitations of our stack (TypeScript, Node.js, etc.) did you run into?
Most of the code written in this phase should be considered as throw-away prototype, it does not necessary have to follow our required practices for testing, docs, etc. Of course, this does not mean you should test everything manually - quite often you will need at least some tests to save you time and prove that your design is easy to test.
The outcome of this step should be a detailed design/implementation proposal in a written form (think about RFCs), posted e.g. in a new GitHub issue and/or as a blog post. This proposal should start and facilitate a high-level discussion about the direction we want to take.
Any code and pull requests created during this phase must be thrown away, unless they are already well-implemented and small enough for easy review. (Which most likely means the person doing the experiment was doing work that's out of scope.)
Based on the agreed-upon design and the prototype code, split the necessary changes into a series of small incremental pull requests.
Each increment should be developed according to our best practices: test-first development, honor all style conventions, include TSDocs, etc.
Each pull request should be small enough to enable meaningful reviews and make it easy to perform large structural changes when requested by reviewers (the smaller the patch is, the less code needs to be reworked).