|
1 | 1 | # Contributing code to matrix-js-sdk
|
2 | 2 |
|
3 |
| -matrix-js-sdk follows the same pattern as https://github.com/vector-im/element-web/blob/develop/CONTRIBUTING.md |
| 3 | +Everyone is welcome to contribute code to matrix-js-sdk, provided that they are |
| 4 | +willing to license their contributions under the same license as the project |
| 5 | +itself. We follow a simple 'inbound=outbound' model for contributions: the act |
| 6 | +of submitting an 'inbound' contribution means that the contributor agrees to |
| 7 | +license the code under the same terms as the project's overall 'outbound' |
| 8 | +license - in this case, Apache Software License v2 (see |
| 9 | +[LICENSE](LICENSE)). |
| 10 | + |
| 11 | +## How to contribute |
| 12 | + |
| 13 | +The preferred and easiest way to contribute changes to the project is to fork |
| 14 | +it on github, and then create a pull request to ask us to pull your changes |
| 15 | +into our repo (https://help.github.com/articles/using-pull-requests/) |
| 16 | + |
| 17 | +We use GitHub's pull request workflow to review the contribution, and either |
| 18 | +ask you to make any refinements needed or merge it and make them ourselves. |
| 19 | + |
| 20 | +Your PR should have a title that describes what change is being made. This |
| 21 | +is used for the text in the Changelog entry by default (see below), so a good |
| 22 | +title will tell a user succinctly what change is being made. "Fix bug where |
| 23 | +cows had five legs" and, "Add support for miniature horses" are examples of good |
| 24 | +titles. Don't include an issue number here: that belongs in the description. |
| 25 | +Definitely don't use the GitHub default of "Update file.ts". |
| 26 | + |
| 27 | +As for your PR description, it should include these things: |
| 28 | + |
| 29 | +- References to any bugs fixed by the change (in GitHub's `Fixes` notation) |
| 30 | +- Describe the why and what is changing in the PR description so it's easy for |
| 31 | + onlookers and reviewers to onboard and context switch. This information is |
| 32 | + also helpful when we come back to look at this in 6 months and ask "why did |
| 33 | + we do it like that?" we have a chance of finding out. |
| 34 | + - Why didn't it work before? Why does it work now? What use cases does it |
| 35 | + unlock? |
| 36 | + - If you find yourself adding information on how the code works or why you |
| 37 | + chose to do it the way you did, make sure this information is instead |
| 38 | + written as comments in the code itself. |
| 39 | + - Sometimes a PR can change considerably as it is developed. In this case, |
| 40 | + the description should be updated to reflect the most recent state of |
| 41 | + the PR. (It can be helpful to retain the old content under a suitable |
| 42 | + heading, for additional context.) |
| 43 | +- Include a step-by-step testing strategy so that a reviewer can check out the |
| 44 | + code locally and easily get to the point of testing your change. |
| 45 | +- Add comments to the diff for the reviewer that might help them to understand |
| 46 | + why the change is necessary or how they might better understand and review it. |
| 47 | + |
| 48 | +### Changelogs |
| 49 | + |
| 50 | +There's no need to manually add Changelog entries: we use information in the |
| 51 | +pull request to populate the information that goes into the changelogs our |
| 52 | +users see, both for Element Web itself and other projects on which it is based. |
| 53 | +This is picked up from both labels on the pull request and the `Notes:` |
| 54 | +annotation in the description. By default, the PR title will be used for the |
| 55 | +changelog entry, but you can specify more options, as follows. |
| 56 | + |
| 57 | +To add a longer, more detailed description of the change for the changelog: |
| 58 | + |
| 59 | +_Fix llama herding bug_ |
| 60 | + |
| 61 | +``` |
| 62 | +Notes: Fix a bug (https://github.com/matrix-org/notaproject/issues/123) where the 'Herd' button would not herd more than 8 Llamas if the moon was in the waxing gibbous phase |
| 63 | +``` |
| 64 | + |
| 65 | +For some PRs, it's not useful to have an entry in the user-facing changelog (this is |
| 66 | +the default for PRs labelled with `T-Task`): |
| 67 | + |
| 68 | +_Remove outdated comment from `Ungulates.ts`_ |
| 69 | + |
| 70 | +``` |
| 71 | +Notes: none |
| 72 | +``` |
| 73 | + |
| 74 | +Sometimes, you're fixing a bug in a downstream project, in which case you want |
| 75 | +an entry in that project's changelog. You can do that too: |
| 76 | + |
| 77 | +_Fix another herding bug_ |
| 78 | + |
| 79 | +``` |
| 80 | +Notes: Fix a bug where the `herd()` function would only work on Tuesdays |
| 81 | +element-web notes: Fix a bug where the 'Herd' button only worked on Tuesdays |
| 82 | +``` |
| 83 | + |
| 84 | +This example is for Element Web. You can specify: |
| 85 | + |
| 86 | +- element-web |
| 87 | +- element-desktop |
| 88 | + |
| 89 | +If your PR introduces a breaking change, use the `Notes` section in the same |
| 90 | +way, additionally adding the `X-Breaking-Change` label (see below). There's no need |
| 91 | +to specify in the notes that it's a breaking change - this will be added |
| 92 | +automatically based on the label - but remember to tell the developer how to |
| 93 | +migrate: |
| 94 | + |
| 95 | +_Remove legacy class_ |
| 96 | + |
| 97 | +``` |
| 98 | +Notes: Remove legacy `Camelopard` class. `Giraffe` should be used instead. |
| 99 | +``` |
| 100 | + |
| 101 | +Other metadata can be added using labels. |
| 102 | + |
| 103 | +- `X-Breaking-Change`: A breaking change - adding this label will mean the change causes a _major_ version bump. |
| 104 | +- `T-Enhancement`: A new feature - adding this label will mean the change causes a _minor_ version bump. |
| 105 | +- `T-Defect`: A bug fix (in either code or docs). |
| 106 | +- `T-Task`: No user-facing changes, eg. code comments, CI fixes, refactors or tests. Won't have a changelog entry unless you specify one. |
| 107 | + |
| 108 | +If you don't have permission to add labels, your PR reviewer(s) can work with you |
| 109 | +to add them: ask in the PR description or comments. |
| 110 | + |
| 111 | +We use continuous integration, and all pull requests get automatically tested: |
| 112 | +if your change breaks the build, then the PR will show that there are failed |
| 113 | +checks, so please check back after a few minutes. |
| 114 | + |
| 115 | +## Tests |
| 116 | + |
| 117 | +Your PR should include tests. |
| 118 | + |
| 119 | +For new user facing features in `matrix-js-sdk`, you |
| 120 | +must include comprehensive unit tests written in Jest. |
| 121 | +The existing tests can be found under `spec/unit` |
| 122 | + |
| 123 | +It's good practice to write tests alongside the code as it ensures the code is testable from |
| 124 | +the start, and gives you a fast feedback loop while you're developing the |
| 125 | +functionality. Unit tests are necessary even for bug fixes. |
| 126 | + |
| 127 | +When writing unit tests, please aim for a high level of test coverage |
| 128 | +for new code - 80% or greater. If you cannot achieve that, please document |
| 129 | +why it's not possible in your PR. |
| 130 | + |
| 131 | +Tests validate that your change works as intended and also document |
| 132 | +concisely what is being changed. Ideally, your new tests fail |
| 133 | +prior to your change, and succeed once it has been applied. You may |
| 134 | +find this simpler to achieve if you write the tests first. |
| 135 | + |
| 136 | +If you're spiking some code that's experimental and not being used to support |
| 137 | +production features, exceptions can be made to requirements for tests. |
| 138 | +Note that tests will still be required in order to ship the feature, and it's |
| 139 | +strongly encouraged to think about tests early in the process, as adding |
| 140 | +tests later will become progressively more difficult. |
| 141 | + |
| 142 | +If you're not sure how to approach writing tests for your change, ask for help |
| 143 | +in [#element-dev](https://matrix.to/#/#element-dev:matrix.org). |
| 144 | + |
| 145 | +## Code style |
| 146 | + |
| 147 | +Code style is documented in [code_style.md](./code_style.md). |
| 148 | +Contributors are encouraged to it and follow the principles set out there. |
| 149 | + |
| 150 | +Please ensure your changes match the cosmetic style of the existing project, |
| 151 | +and **_never_** mix cosmetic and functional changes in the same commit, as it |
| 152 | +makes it horribly hard to review otherwise. |
| 153 | + |
| 154 | +## Sign off |
| 155 | + |
| 156 | +In order to have a concrete record that your contribution is intentional |
| 157 | +and you agree to license it under the same terms as the project's license, we've |
| 158 | +adopted the same lightweight approach that the Linux Kernel |
| 159 | +(https://www.kernel.org/doc/html/latest/process/submitting-patches.html), Docker |
| 160 | +(https://github.com/docker/docker/blob/master/CONTRIBUTING.md), and many other |
| 161 | +projects use: the DCO (Developer Certificate of Origin: |
| 162 | +http://developercertificate.org/). This is a simple declaration that you wrote |
| 163 | +the contribution or otherwise have the right to contribute it to Matrix: |
| 164 | + |
| 165 | +``` |
| 166 | +Developer Certificate of Origin |
| 167 | +Version 1.1 |
| 168 | +
|
| 169 | +Copyright (C) 2004, 2006 The Linux Foundation and its contributors. |
| 170 | +660 York Street, Suite 102, |
| 171 | +San Francisco, CA 94110 USA |
| 172 | +
|
| 173 | +Everyone is permitted to copy and distribute verbatim copies of this |
| 174 | +license document, but changing it is not allowed. |
| 175 | +
|
| 176 | +Developer's Certificate of Origin 1.1 |
| 177 | +
|
| 178 | +By making a contribution to this project, I certify that: |
| 179 | +
|
| 180 | +(a) The contribution was created in whole or in part by me and I |
| 181 | + have the right to submit it under the open source license |
| 182 | + indicated in the file; or |
| 183 | +
|
| 184 | +(b) The contribution is based upon previous work that, to the best |
| 185 | + of my knowledge, is covered under an appropriate open source |
| 186 | + license and I have the right under that license to submit that |
| 187 | + work with modifications, whether created in whole or in part |
| 188 | + by me, under the same open source license (unless I am |
| 189 | + permitted to submit under a different license), as indicated |
| 190 | + in the file; or |
| 191 | +
|
| 192 | +(c) The contribution was provided directly to me by some other |
| 193 | + person who certified (a), (b) or (c) and I have not modified |
| 194 | + it. |
| 195 | +
|
| 196 | +(d) I understand and agree that this project and the contribution |
| 197 | + are public and that a record of the contribution (including all |
| 198 | + personal information I submit with it, including my sign-off) is |
| 199 | + maintained indefinitely and may be redistributed consistent with |
| 200 | + this project or the open source license(s) involved. |
| 201 | +``` |
| 202 | + |
| 203 | +If you agree to this for your contribution, then all that's needed is to |
| 204 | +include the line in your commit or pull request comment: |
| 205 | + |
| 206 | +``` |
| 207 | +Signed-off-by: Your Name <[email protected]> |
| 208 | +``` |
| 209 | + |
| 210 | +We accept contributions under a legally identifiable name, such as your name on |
| 211 | +government documentation or common-law names (names claimed by legitimate usage |
| 212 | +or repute). Unfortunately, we cannot accept anonymous contributions at this |
| 213 | +time. |
| 214 | + |
| 215 | +Git allows you to add this signoff automatically when using the `-s` flag to |
| 216 | +`git commit`, which uses the name and email set in your `user.name` and |
| 217 | +`user.email` git configs. |
| 218 | + |
| 219 | +If you forgot to sign off your commits before making your pull request and are |
| 220 | +on Git 2.17+ you can mass signoff using rebase: |
| 221 | + |
| 222 | +``` |
| 223 | +git rebase --signoff origin/develop |
| 224 | +``` |
| 225 | + |
| 226 | +# Review expectations |
| 227 | + |
| 228 | +See https://github.com/vector-im/element-meta/wiki/Review-process |
| 229 | + |
| 230 | +# Merge Strategy |
| 231 | + |
| 232 | +The preferred method for merging pull requests is squash merging to keep the |
| 233 | +commit history trim, but it is up to the discretion of the team member merging |
| 234 | +the change. We do not support rebase merges due to `allchange` being unable to |
| 235 | +handle them. When merging make sure to leave the default commit title, or |
| 236 | +at least leave the PR number at the end in brackets like by default. |
| 237 | +When stacking pull requests, you may wish to do the following: |
| 238 | + |
| 239 | +1. Branch from develop to your branch (branch1), push commits onto it and open a pull request |
| 240 | +2. Branch from your base branch (branch1) to your work branch (branch2), push commits and open a pull request configuring the base to be branch1, saying in the description that it is based on your other PR. |
| 241 | +3. Merge the first PR using a merge commit otherwise your stacked PR will need a rebase. Github will automatically adjust the base branch of your other PR to be develop. |
0 commit comments