-
Notifications
You must be signed in to change notification settings - Fork 102
Pull Requests
You can create a PR at any time for an existing issue. If the work is not yet in a position to be reviewed then ensure that you create the PR as a draft.
A PR should have exactly one concern (i.e. a feature or a bug). A PR that addresses more than one concern should ideally be split into multiple PRs.
When creating the PR, you will be presented with our current template; it is advisable to fill this template in as detailed as possible, and to the best of your ability. This will allow for a smoother review process, and high probability of your branch being merged.
You should also assign the PR to yourself, add the appropriate labels, link any relevant issues to be closed and if known, request a review of a maintainer.
It is important to keep the formatting of PRs clean and consistent. In addition to generally following the template, we also use the following rules:
- PR titles must be in the format
type: the title of the PR.- All words must be lowercase except proper nouns (e.g. "Electron", "Stronghold") and acronyms (e.g. "API", "iOS").
- All code or package names should be formatted as
inline code. This is best for things like dependencies as specific variable names are subject to change.
- Instructional text (like this) in the template should be removed in PR descriptions.
Please ensure all PRs have a type label, additional labels can be added where deemed appropriate.
-
feat: Introduces a new feature
-
enhancement: Enhancement of an existing feature -
refactor: Improvements made to existing code -
chore: Modifies documentation, cleaning dependencies, or other housekeeping-like tasks -
fix: Bug, error, or failure that has been fixed
-
do not merge: Do not merge into any working branches
Before requesting a review please make sure:
- You have completed the task defined in the issue.
- If you've added code that should be tested, add tests.
- Ensure the test suite passes.
- Make sure your code lints.
- A PR should have exactly one concern (i.e. one feature or one bug).
- All code should follow the coding guide.
Once your PR fulfils the above criteria, you are free to request a review from one of the maintainers.
There are no limitations on who can review a PR, the more eyes on the code the better. All maintainers, Firefly developers and contributors are encouraged to review as many PRs as possible, as well as community members.
At a minimum, it is advised to do the following during a review:
- Read the lined issues and PR description to get context for the review
- Pull the branch to your local machine and build the application
- Manually test the application, focusing on any new features or fixes that have been added
- Review the code that has been added or changed
- Add feedback in the form of comments, utilising the GitHub review process where possible
You may also want to:
- Speak to the author, and walk through the code together
- Run tests and linting checks locally (although these are done in GitHub anyway)
- Suggest changes to be made by the author
- Commit your own small changes if you have the time
Conditions for PRs to be merged are dependent on the target branch.
Before a PR can be merged into the develop branch, it must satisfy the following conditions:
- At least one approval review
- Zero unresolved comments
- No merge conflicts with the target branch
- Status checks:
- Format and linting tests pass on Rust and JS/TS files
- Unit tests pass
- If introducing a dependency then Snyk tests pass*
* If your PR is based off of a forked repository, there is no way to test the Snyk continuous integration workflow as it needs an API key. Once your PR has been approved, you must create an intermediary branch on the main repository titled snyk/your-branch-here so that the checks can be performed. Once that passes, then your branch may finally be merged into develop.
Before a PR can be merged into the develop branch, it must satisfy the following conditions:
- At least one approval review
- Zero unresolved comments
- No merge conflicts with the target branch
- Status checks:
- Format and linting tests pass on Rust and JS/TS files
- Unit tests pass
- Snyk tests pass
To merge our PRs we are using the Squash and merge option in GitHub.
When you select the Squash and merge option on a pull request on GitHub, the pull request's commits are squashed into a single commit. Instead of seeing all of a contributor's individual commits from a topic branch, the commits are combined into one commit and merged into the default branch. Pull requests with squashed commits are merged using the fast-forward option.
To squash and merge pull requests, you must have write permissions in the repository, and the repository must allow squash merging.