-
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.
- Go to: https://github.com/kframework/k
- Click the "Fork" button in the upper right, then select your personal account.
- In your git console, run
git clonewith the argument in the right sidebar on github. For example, Dwight runsgit clone https://github.com/dwightguth/k.git.
- Once you have a working copy, make your changes, committing as necessary. Then push to your fork. Make sure if you merge changes from another branch that you rebase in order to keep the history clean.
- Log into github and click "Pull request" next to the line that says "This branch is commits ahead and commits behind master".
- Make sure you are submitting the correct change, type up a description for it, then submit it. This automatically emails everyone watching the K framework repository except yourself.
- At this point it becomes the responsibility of reviewers to comment on your change and either approve it or request modifications. If they do the latter, you can update the pull request simply by pushing further changes on the same branch to your fork. If they do the latter, you are free to click the "Merge pull request" button on the bottom of the pull request screen to automatically merge and close the pull request.
- Note that any push made to the same branch after the pull request is opened but before it is closed will add that commit to the pull request. To continue working on a different change during that time period, you must work on a branch.
- Create a short-term feature branch in your fork by running the command
git branch <branch_name> <commit_id_to_branch_from>. For example to create a branch named "foo" from the commit immediately prior to the most recent, you might rungit branch foo HEAD^. - Switch to the new branch by running
git checkout <branch_name>. - Start making your new change. When it comes time to perform a pull request, push the changes upstream to the branch you created by running
git push origin <branch_name>. - Proceed from section "Create the pull request"
- (do this once) run
git remote add upstream <github_url_for_main_repo>. This adds a remote repository to your local repository for the kframework/k repository. - Run
git pull --rebase upstream <branch_name>to sync a particular branch with the main repository. Do this if you are creating a new pull request and see that it contains commits that are not yours.
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].