-
Notifications
You must be signed in to change notification settings - Fork 0
Code Reviews
In order to enforce good code quality in our repository, we will be implementing a code review policy for all commits to the K framework repository's master branch. The purpose of this document is to outline and explain this policy as unambiguously as possible in order to answer questions that might arise in the execution of code reviews.
All code committed to the "master" branch of the K framework repository must be code reviewed. It is up to you to decide how best to allocate your time and effort in order to make this happen, however, be aware that only clean code will be allowed to be pushed to master, and some workflows will make that easier than others. For example, all commits to a long-running feature branch must be code reviewed or we will automatically reject the pull request to merge the branch into "master". So you should consider committing to that branch on a fork and sending a pull request out.
- Commits to a fork of a long-running feature branch that you intend to eventually merge into master
- Commits to a short-running feature branch that you will merge quickly
- Commits to a fork of the k framework repository that you would like to pull into the main repository
In order to commit your code to master, you must get it reviewed first. To do this, commit it to someplace that is not subject to code reviews (i.e. either a short-term feature branch or a fork of the main repository). Then log in to github and create a pull request.
All code must be reviewed by two people. An approved "senior engineer", and another team member who is expected to own responsibility for that section of the code base. For example, if someone were to commit to the Java Rewrite Engine, it is expected that they get both a senior engineer and someone else familiar with the Java Rewrite Engine to review the change.
The senior engineer reviewing the code ultimately has final veto regarding whether the code goes out. If they say it's fine, it's fine. If they say it's not fine, it's not fine. If they tell you to change something, you either change it or attempt to persuade them it should not be changed, and if you can't persuade them, you need to bring it up in a larger design review and reach group consensus before you can check the code in.
The senior engineer is expected to be familiar enough with code quality to understand whether the code is ready to go out. This means documentation, test coverage, style and cleanness of the code, and design considerations in the architecture of the change. Only engineers on the team who can demonstrate proficiency in each of these areas will be eligible to be considered as senior engineers.
The second reviewer is expected to understand the change. If they have feedback, great. The more we can improve the code, the better. But their purpose in the review process is a little different. They are present in order to ensure that no code is written which is not understood by the other members of the group who need to be able to maintain a change. In order to do this, they must be able to explain in very clear terms what the change is doing, why, and how. If they cannot do this easily, the primary owner of the change needs to modify their commit to make it easier to understand these things. It is not the responsibility of the owner to explain their code. It is their responsibility to make it self-explanatory. It is also the responsibility of the senior engineer to not approve any changes which do not meet this minimum requirement between the owner and the second reviewer.
In order to speed up the development process, we will not at first be preventing users from committing to the k framework's master branch. Instead, we will have a mechanism of punitive enforcement where users who do not obey the policy as laid out above will be subject to processes which ensure the eventual enforcement of the policy.
Users who commit code to the master branch or a long-running feature branch which has not been approved by the senior engineer and understood by a second reviewer have committed what for the purposes of this policy we will call a "violation". Users who commit a violation but immediately remove the offending change from the respective branch have "self-corrected" their violation. Users can commit violations on accident without penalty as long as they immediately self-correct the violation.
Users who commit violations purposely or who do not self-correct their violation immediately are put on probation. Probations last two weeks. A user who commits a violation while on probation and does not immediately self-correct will have the ability to commit to the main repository removed. Users who would otherwise go on probation a third time will also have their access removed.
Loss of commit access to the repository will not significantly impact your productivity. Continue to make changes on your own fork of the repository and submit pull requests. If they are approved, the senior engineer with commit access will complete the pull for you.
This policy is intended to improve the code quality of all code in the master K framework repository. By doing this, we increase efficiency in the long run by preventing long delays that arise from having to clean up, refactor, and fix bugs in code. Consistently industry case studies show that productivity is increased when the code base is in a clean state all the time, and evolves by moving incrementally from clean state to clean state. By causing each commit to be reviewed by someone familiar with the component that is changed, we also ensure any one team member may leave the team without impacting the overall understanding of the code base.
I understand that this transition is complex and requires a lot of details to work correctly. So if you have questions, please email me at [email protected]. I will attempt to respond promptly.