-
Notifications
You must be signed in to change notification settings - Fork 61
Contributing and Code Reviewing Guide
- Create a new branch containing the changes you want to merge. If you do have commit rights to the main K repository, you may use it instead of your own fork. This allows others to help with fixes to your code instead of merely pointing them out.
- If you know the right person to review your pull request, assign them to the PR. There should be one main reviewer (who will be responsible for merging the pull request) but you can ask for an opinion from multiple people.
- Add the ready-for-review tag.
- Wait for the review. If the person doesn't make a complete review (marked by the reviewed tag) within two days, feel free to ask someone else.
- Once you have addressed, either through code or comments, the raised issues, re-add the ready-for-review tag and assign the PR back to the reviewer (i.e., go to step 3).
- Do not merge the pull request yourself. The reviewer will do it for you when they are content with the changes.
First, read the in-depth description below this paragraph for details on how to do a good code review. Once you have read and understood that, follow the steps below:
- You will be asked to do a review by being either assigned or "@" mentioned on a pull request which is ready-for-review. If there are multiple people asked to review, decide on a main reviewer and assign her to the pull request. If you are the main reviewer but cannot do the review in the next couple of days, please tell the contributor as soon as possible. If you think you may not be qualified to review those changes, ask someone else either to take over the PR or give advice.
- Make suggestions or even fix small things directly if you have time.
- Once done, remove the ready-for-review tag and replace it with reviewed, and assign the contributor to the PR.
- Once the person has integrated the changes (and has marked the pull request with ready-for-review), if there is anything else that you are not content with, mention it and flip the ready-for-review/reviewed flags again (essentially go to step 1).
- Once you are content with the changes, merge the pull request yourself. This way you are both saying that you are content with the changes, and you are putting permanent marker in the history (the merge commit) taking responsibility for the quality of the pull request.
Note: As the main reviewer of a PR, you have both responsibility and power. You are responsible for the quality of the PR so you decide what is still needed for the PR to be merged, and when to merge it. This holds even when there are multiple reviewers with possibly-diverging opinions. You should make decisions on which suggestions need to be incorporated in the PR, and which can be postponed (with adding an issue) or dropped. As long as the PR is "ready-for-review", you are also responsible for its timeliness. If other reviewers are late, and their opinion is important for this PR, you are responsible for encouraging them to send in their feedback. If they are not available, you can also decide to go ahead without their input. Once the PR is tagged with "reviewed", its timeliness is again out of your hands.
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
- Commits to be code reviewed should be tested.
- Commits to be code reviewed which leave some specific piece of functionality undone for later should have that functionality labelled with a TODO.
- The reviewer can reserve the right to categorically reject a change if they believe it is too hard to understand because of the way it intermixes multiple conceptually unique modifications to the code.
- Commits to be code reviewed should do their best to strive to be clean according to the Code Quality Principles.
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.
In order to ensure that lack of developer interaction does not bottleneck the productivity of developers, we are instituting a policy that all code reviews in which you are explicitly asked to review should either be reviewed within 2 business days, or else you should explain in a comment on the pull request why you are not able to complete the pull request right away and give a date you plan to get to it. If you are unable to get to it at that date, you must continue to provide updates until you are able to complete the review. For every business day up to five business days after the deadline that you delay the review, Dwight will delay the same number of days the next time you ask him to write or look at code for you.
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.
That said, it is not the senior engineer's responsibility to make design decisions regarding system behavior. In other words, if you can demonstrate to me that you are able to provide a clean design and implementation of the choices you made in order to pursue your own research, the senior engineer cannot force you to change the direction you are taking your code. I (Dwight) ran into situations recently where I overstepped my authority, so I wanted to formalize this. There is a difference between code that does something uncleanly and code that cleanly does something slightly different than you think it should do, and we need to respect that distinction. So if you feel the senior engineer is overstepping their bounds, you should direct them to review this policy and if you cannot reach agreement following that point, you should bring Grigore into the discussion to arbitrate.
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.
We understand that this transition is complex and requires a lot of details to work correctly. So if you have questions, please email us at [email protected] and [email protected].