-
Notifications
You must be signed in to change notification settings - Fork 230
RMG Contributor Guidelines
The purpose of this document is to formalize the guidelines for contributing to RMG. RMG is an open source project built and maintained by a team of developers. Anyone can contribute to RMG, whether to the code, documentation, RMG-database, or general ideas.
RMG is somewhat unique in that the primary development team is composed of PhD students and postdocs, which results in frequent turnover. Thus, these contributor guidelines are intended to make it easier to start contributing while maintaining code quality. For new developers, the best way to start contributing is by reviewing pull requests. This will help you become more familiar with the codebase and the process of making changes.
The general process and responsibilities for submitting contributions are summarized in the following table. The person making the changes and pull request is the Author, and the person providing feedback on the pull request is the Reviewer. Once the pull request has been approved by the Reviewer, then the request can then be merged. The Merger can be anyone with push permissions to the master branch, aside from the Author. New Mergers can be nominated and approved by the committee of project owners (current owners are William Green, Richard West, Nathan Yee, Kehang Han, and Max Liu).
| Step | Task | Person Responsible |
|---|---|---|
| 0 | Propose changes and get approval (for big changes) | Author |
| 1 | Implement and test desired changes locally | Author |
| 2 | Make pull request when ready | Author |
| 3 | Request reviewer | Author |
| 4 | Review changes and provide feedback | Reviewer |
| 5 | Address reviewer feedback | Author |
| 6 | Confirm that all checks pass and approve request | Reviewer |
| 7 | Merge pull request | Merger |
Steps 4 and 5 may be repeated as needed until the Reviewer is completely satisfied. The following section provides further details on required checks and suggested guidelines.
The primary method to contribute to the RMG codebase is via pull requests. Once a contributor has developed new features or fixes and wishes to merge them into the official codebase, they may create a pull request on GitHub to merge their changes into the master branch.
There are a few mandatory checks that must pass for new pull requests. These should be checked by the Author before making the pull request, and confirmed by the Reviewer before approving the pull request.
-
Unit tests:
The pull request must pass the test suite within the RMG-Py or RMG-database repository, which is run automatically by TravisCI. -
Regression tests:
The pull request must pass the integration test RMG-tests, which is also run automatically by TravisCI. Anytime TravisCI cannot complete RMG-tests due to time limitation, the Author should run RMG-tests locally and explain any substantial result differences caused by the new branch that the pull request is made for. -
Code coverage:
The pull request should not reduce code coverage by unit tests, which is checked automatically by CodeCov. This does not apply for RMG-database.
Aside from the mandatory requirements, there are a number of other guidelines to ensure that the pull request process proceeds smoothly.
- For big changes/features, the Author should first propose the changes before proceeding, either at an RMG meeting or online (GitHub, Gitter, Slack, email), and get approval from the committee of project owners that the implementation approach is beneficial for RMG and compatible with code organization design.
- The Author should make sure that the changes are sufficiently polished and tested before making the pull request. The effect of the changes on computation time should also be evaluated to avoid unexpected or unnecessary performance reduction.
- A pull request should include a cohesive set of commits implementing a single patch or feature.
- Try to keep pull requests small; it makes reviewing and merging much smoother.
- However, do not split pull requests in a way that breaks functionality.
- Explain the reason for the pull request and an overview of what the changes are. Reference any issues which the pull request fixes.
- It is the responsibility of the Author to find a reviewer if no one volunteers.
- The Author should ensure that the branch is up to date by rebasing onto the master branch. In particular, this must be done before the pull request is merged.
- New contributors are encouraged to review pull requests to gain experience. Depending on the complexity of the changes, the merger should decide whether a second review by a more experienced contributor is necessary.
- At least one other contributor must review and approve the pull request. If there are multiple authors, the pull request should be reviewed by a non-author if possible.
- If there are any disagreements (e.g., default settings of a new feature, code style, etc.), the requestor and reviewer should attempt to reach a consensus on the desired path forward. If a decision cannot be reached, the question can be discussed with a committee of at least two uninvolved project owners.
- Any interested person should also contribute to discussion regarding a pull request. They are encouraged to contribute within a reasonable timeframe, but also have the right to request additional time for discussion.
- Reviewer checklist:
- Does the code work?
- Does the code make sense?
- Are the changes unit-tested?
- Is the code sufficiently commented?
- Are documentation changes necessary?
- Are the commit messages descriptive?
- Merger should decide if a second reviewer is needed based on experience of the first reviewer and complexity of the pull request.
- Pull requests may be merged by anyone who has push access to the master branch, aside from the Author.
- The pull request branch should be rebased on top of the master branch before merging to ensure a clean history. If the branch is not up to date, the Author should rebase it.
- “Merge pull request” option is the preferred way to merge pull requests. “Rebase and merge” option is acceptable if the pull request only contains one (1) commit. “Squash and merge” is never allowed.
Having lots of unused and potentially broken code leads to difficulty developing and using the software. In addition, removing parts of the code which other people depend upon can be detrimental to development and usage as well. A good deprecation procedure can help find a happy optimum.
If you find code which you think is no longer working, you can create an issue about it, using the tag "Type: deprecation" and/or you can add a deprecation warning to the function or method. A good deprecation warning describes the functionality being deprecated and the next release of RMG which the method/functionality might be removed in.
warnings.warn("The option saveRestartPeriod is no longer supported"\
" and may be removed in version 2.3.", DeprecationWarning)
Remember to import the warnings package in the file you are adding the warning to.
This deprecation warning serves as a reminder for people who use this method to mention, on GitHub, that they are potentially deprecating an important functionality for you, which can be the start of a conversation to find the optimum way to improve the code, which could involve refactoring the code, still deprecating the code, or leaving the code as is.
If no one raises issue with the change after 6 months, someone can make a pull request removing the method/functionality.