CONTRIBUTING.md: move content from DocuWiki #2699
Conversation
c0e0b94 to
bb7213f
Compare
This content was at https://mixxx.org/wiki/doku.php/contribution_guidelines but not many developers saw it. The conventional place for such documents is a CONTRIBUTING.md file in the Git repository.
|
Thank you. However I think the CONTRIBUTING.md should be short and concise, because nobody will read it otherwise. We should add a Quickstart or 'Golden Rules' section at the top with a list of bullet points, and move most of the detailed info to the Wiki or another document. |
|
I disagree. Take a look at Rust's CONTRIBUTING.md for example. |
| # Mixxx Contribution Guidelines # | ||
| Thank you for contributing to [Mixxx](https://mixxx.org/)! Your work helps DJs all over the world! We are global, all volunteer team that works by consensus and we are excited to have you join us. This document specifies technical aspects of our workflow. For social aspects please refer to [CODE_OF_CONDUCT.md](https://github.com/mixxxdj/mixxx/blob/master/CODE_OF_CONDUCT.md) in this repository. We encourage you to introduce yourself on our [Zulip chat](https://mixxx.zulipchat.com/) before starting to contribute code to Mixxx. | ||
|
|
||
| Table of Contents |
There was a problem hiding this comment.
I think this should be a heading
| 1. [Orientation](#Orientation) | ||
| 1. [Git Repositories](#Git-Repositories) | ||
| 2. [Git Workflow](#Git-Workflow) | ||
| 1. [All Contributors](#All-Contributors) | ||
| 2. [Core Team](#Core-Team) |
There was a problem hiding this comment.
The numbers here don't make much sense to me
There was a problem hiding this comment.
Are you suggesting to change this to an unordered list? Tables of content are usually ordered lists.
There was a problem hiding this comment.
On github mostly not, from what I've seen, for example:
https://github.com/sghpjuikit/player/blob/master/README.md
https://github.com/sharkdp/bat/blob/master/README.md
So, yes, I would change this to an unordered list, and maybe even remove the heading "Table of Contents" - it is self-evident.
| * Each feature/bug fix should be done on its own Git branch so they can be reviewed and merged independently. Refer to [Using Git](https://mixxx.org/wiki/doku.php/using_git) for how to do this. Please ask for help on [Zulip](https://mixxx.zulipchat.com/) if you have questions about using Git after reading that page. | ||
| * Commits should be as small as they can while still building. The smaller the commit, the easier it is to review. It also makes it easier to revert if it is later identified as the source of a bug. If you have lots of changes that you need to commit, a [GUI Git client](https://git-scm.com/downloads/guis) can be helpful for picking out specific changes for multiple small commits. | ||
| * Every commit should build. This is important so [git bisect](https://git-scm.com/book/en/v2/Git-Tools-Debugging-with-Git#_binary_search) works. | ||
| * Commit messages should succinctly describe what is changed in that commit and why. Lines should wrap at 72 characters so they show fully in GitHub and other Git tools. For example, this is a good commit message: | ||
|
|
||
| ``` | ||
| DlgPrefEffects: add QListWidget to set order of chains | ||
|
|
||
| This order will soon be used by new ControlObjects to load them | ||
| from controllers. | ||
| ``` | ||
|
|
||
| This is not a good commit message: | ||
|
|
||
| ``` | ||
| address comments from PR review | ||
| ``` | ||
|
|
||
| Neither is this: | ||
|
|
||
| ``` | ||
| fix a bug with quantize while the deck is playing and master sync is enabled and an effect unit is on the deck while the user is turning an EQ knob | ||
| ``` | ||
|
|
||
| Refer to [How to Write a Git Commit Message](https://chris.beams.io/posts/git-commit/) for more details. | ||
|
|
||
| * Install [pre-commit](https://pre-commit.com/#install) to automatically ensure that your commits comply with our code style for both C++ and JavaScript. Note this is currently not working on Windows. This saves time reviewing so we don't have to point out nitpicky style issues. Once you have pre-commit installed on your computer, set it up in your local Git repository: | ||
|
|
||
| ``` | ||
| cd /path/to/your/git/repo | ||
| pre-commit install | ||
| pre-commit install -t pre-push | ||
| ``` | ||
|
|
||
| If you have a problems with a particular hook, you can use the SKIP environment variable to disable hooks: | ||
|
|
||
| ``` | ||
| SKIP=clang-format,end-of-file-fixer git commit | ||
| ``` | ||
|
|
||
| This can also be used to separate logic changes and autoformatting into two subsequent commits. Using the SKIP environment variable is preferable to using `git commit --no-verify` (which also disables the checks) because it won't prevent catching other, unrelated issues. |
There was a problem hiding this comment.
Please divide this into sections, it is rather unwieldy if you are searching for something specific.
E.g. this section could be called "Committing"
| * Low risk bug fixes should be targeted at the stable branch (currently 2.2). However, bug fixes for the stable branches must have a direct impact on users. If you spot a minor bug reading the code or only want to clean up the code, target that at the master or beta branch. | ||
| * Controller mappings should be targeted at the stable branch unless they use features that are new in the beta or master branch. | ||
| * If you are making changes to the GUI with a pull request, please post before and after screenshots of the changes. | ||
| * Please help review other people's pull requests. When others review your pull requests, please return the favor. The continued progress of Mixxx depends on all of us working together. Even if you are not familiar with the area of the code being changed in a pull request, you can be helpful by building the branch, verifying that it works as described, and commenting with feedback about the user experience design. |
There was a problem hiding this comment.
| * Please help review other people's pull requests. When others review your pull requests, please return the favor. The continued progress of Mixxx depends on all of us working together. Even if you are not familiar with the area of the code being changed in a pull request, you can be helpful by building the branch, verifying that it works as described, and commenting with feedback about the user experience design. | |
| * Please help review other people's pull requests. The continued progress of Mixxx depends on all of us working together. Even if you are not familiar with the area of the code being changed in a pull request, you can help by building the code, verifying that it works as described and giving feedback about the user experience. |
I don't like that notion of forced reciprocity here.
| * Controller mappings should be targeted at the stable branch unless they use features that are new in the beta or master branch. | ||
| * If you are making changes to the GUI with a pull request, please post before and after screenshots of the changes. | ||
| * Please help review other people's pull requests. When others review your pull requests, please return the favor. The continued progress of Mixxx depends on all of us working together. Even if you are not familiar with the area of the code being changed in a pull request, you can be helpful by building the branch, verifying that it works as described, and commenting with feedback about the user experience design. | ||
| * If you demonstrate good coding skills, help review pull requests, contribute major features, and show a commitment to Mixxx over time, we may invite you to the core team. |
There was a problem hiding this comment.
| * If you demonstrate good coding skills, help review pull requests, contribute major features, and show a commitment to Mixxx over time, we may invite you to the core team. | |
| If you demonstrate good coding skills, help review pull requests, contribute major features, and show a commitment to Mixxx over time, we may invite you to the core team. |
I think this should be more of a conclusory/bridging paragraph than an extra point
| Mixxx core team members are contributors who have write access to the [upstream mixxxdj repositories](https://github.com/mixxxdj/) on GitHub, access to the Jenkins web interface for the build servers, and access to the private Zulip stream for the core team. | ||
|
|
||
| * Enable [two-factor authentication (2FA)](https://help.github.com/en/github/authenticating-to-github/securing-your-account-with-two-factor-authentication-2fa) for your GitHub account. | ||
| * _Never_ force push to an upstream repository (mixxxdj). If you encounter an error from Git saying you would need to force push, stop what you are doing and discuss the situation on Zulip. |
There was a problem hiding this comment.
You can enforce that in the repo settings, no need for an extra statement here.
There was a problem hiding this comment.
Oh, good to know. I think only @rryan has permission to change the repo settings.
| * You may merge someone else's pull request as the only reviewer if no other contributors have expressed concerns about the changes or said they want to review the code. Please do not merge pull requests immediately; allow at least a day or two for others to comment. Remember we are all volunteers and cannot respond to everything immediately. | ||
| * If there is disagreement about changes in a pull request, do not merge it until a consensus has been reached. | ||
| * Check CI to ensure builds work and tests pass before merging. If CI timed out, either manually restart it or build the branch and run the tests locally before merging. | ||
| * When you merge a pull request to a stable branch, merge the stable branch to the beta branch afterwards. If you merge a pull request to a beta branch, merge the beta branch to master afterwards. When backporting, cherry-pick or rebase rather than merge. |
There was a problem hiding this comment.
Section: Merging Pull Requests
| * _Never_ force push to an upstream repository (mixxxdj). If you encounter an error from Git saying you would need to force push, stop what you are doing and discuss the situation on Zulip. | ||
| * Only push directly to an upstream repository (mixxxdj) for trivial, uncontroversial changes like fixing a typo. | ||
| * All non-trivial contributions should be made with a pull request, just like any other contributor who does not have write access. Do not merge your own pull requests. |
| * Generally, prefer merging to rebasing. Do not rebase unless you have discussed that with whoever is reviewing the pull request. When you rebase a branch with an open pull request, prior review comments made inline in the code on GitHub lose their connection to that spot in the code. If you want to correct minor mistakes with a rebase or `git commit --amend` within a few minutes of pushing commits, that is okay as long as no one has started reviewing those commits yet. | ||
| * If you are helping with someone else's pull request that is not yet merged, open a pull request targeted at their fork. Leave a comment on the upstream pull request (which targets mixxxdj/mixxx) with a link to your pull request so other Mixxx contributors are aware of your changes. | ||
| * Low risk bug fixes should be targeted at the stable branch (currently 2.2). However, bug fixes for the stable branches must have a direct impact on users. If you spot a minor bug reading the code or only want to clean up the code, target that at the master or beta branch. | ||
| * Controller mappings should be targeted at the stable branch unless they use features that are new in the beta or master branch. | ||
| * If you are making changes to the GUI with a pull request, please post before and after screenshots of the changes. | ||
| * Please help review other people's pull requests. When others review your pull requests, please return the favor. The continued progress of Mixxx depends on all of us working together. Even if you are not familiar with the area of the code being changed in a pull request, you can be helpful by building the branch, verifying that it works as described, and commenting with feedback about the user experience design. |
|
I vouch for a section for how to handle review comments, as I did in my collaboration convention: https://xerus2000.github.io/kull/#3-reviewing I am regularly irritated if someone resolves my non-trivial review comment, because imho I should decide whether the person understood me correctly and that comment it actually solved. |
Co-authored-by: Janek <xerusx@pm.me>
Okay, then we should also add something saying that reviewers should resolve comments. |
|
This is the workflow I use and like:
You can have a look at the (unfortunately german) resolved review comments here illustrating that: software-challenge/backend#260 Additional rules that seem sensible:
The important thing is that the contributor refrains from closing non-trivial issues. If I can't rely on that, I regularly look at "resolved" conversations regularly to check whether they are actually resolved. |
|
I would prefere that only the reviewer resolved the conversations. There is already a kind of "resolved" state for the contributor, because the conversation is becoming outdated and it is removed from the diff view. |
|
I'm actually in the other camp. When I look at a PR and see that some if my previous comments have not been resolved yet I assume that the contributer still has stuff to do and the PR is not ready to re-review. Also, if we say that only the original reviewer may resolve issues this will essentially mean that we need to wait until every reviewer has reviewed the PR a second time. That will probably prolong our review times even more. |
This is already the case without the resolve feature. Review, fail, review again approve. I can live with the contributor resolve the comments, however this is an extra click during the second review, and it becomes worse if a third review is required or a second reviewer has done some work. Du to the collapsed state you cannot instantly see if it is an old finished comment or a new just fixed comment. The reviewers needs to open and close many resolved comments to check them. |
I didn't realize we have this requirement. Before I think it was possible to have person A open a PR, person B leaves some review comments, person A makes changes, person C approves and merges. With this system we need to wait for person B to review again, too, before we can merge. |
|
Yes, this was my understanding. |
From the PRs I worked on so far, almost always the original reviewer re-reviewed when I made changes - since review comments can be misunderstood/misinterpreted, I think this is important. |
|
But I don't think the original reviewer should be required to grant explicit approval, i.e. perform a second full review. If all his comments are resolved (at least code-wise) and there is another approval, it should be fine to merge. |
|
There is no IMHO no question about this. |
|
That means we should always use the review mechanism instead of simple comments. If you leave comments that don't necessarily need to be taken care of before merge, you can use "Comment" instead of "approve" or "Request changes". And if you requested changes that are resolved now, you need to explicitly "Approve" them using the review mechanism, not just comment "LGTM" (because else the red cross will persist). I'm okay with that. What about pending reviewers though? There will be a yellow dot if a review was requested but not done. Should we always wait until everyone in the reviewers list approved or commented? If someone contributor requested review by someone but that person does not replay or review (but someone else does), I'd be okay with merging anyway to speed up the process but it might also make sense to wait so that we can use the reviewers list to know if a PR is ready to merge at a glance. |
|
We have the rule that a single LGTM (old) or an "Approved" (new) review is sufficient. Since the reviewer does the merge, we have another safety net. So far, the review request is a kind of ping for review, to me and does not affect the rule above. If a review from one person is desired, we should write it as a comment as well. |
|
This PR is marked as stale because it has been open 90 days with no activity. |
|
I still think we should add a Quickstart section at the top, outlining the most important points for drive-by contributors, each linking to the corresponding verbose section e.g.:
|
|
This PR is marked as stale because it has been open 90 days with no activity. |
|
@Holzhaus: merge? |
|
I still disagree strongly with the wall of text that hides important information. It's too much too to require contributors to read that much IMHO. But it's certainly better than the current situation, so let's merge. |
|
Um, now this was merged without addressing the review comments... |
|
Yes. I'm not happy with CONTRIBUTING.md either, let's do that in other PRs. |
This content was at
https://mixxx.org/wiki/doku.php/contribution_guidelines
but not many developers saw it.
The conventional place for such documents is a CONTRIBUTING.md
file in the Git repository.