From 97df1299568b013f3e53ff3d496a33d06549ec1e Mon Sep 17 00:00:00 2001 From: Michael Reed Date: Mon, 9 Mar 2015 19:20:15 -0400 Subject: CONTRIBUTING.md: Misc improvements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - In general, many sections have been expanded a lot. - Defer to the wiki when it makes sense. - Rewrite a bunch of things to be clearer. There is a lot more verbosity, but clarifying as many items as we can in this document is preferable to it being done later in the issue tracker. (alphabetically sorted) Helped-by: David Granström Helped-by: Florian Walch Helped-by: John Szakmeister Helped-by: Justin M. Keyes Helped-by: Martin Kopischke Helped-by: oni-link [ci skip] --- CONTRIBUTING.md | 248 ++++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 167 insertions(+), 81 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c1b9d50143..5133f50ad0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -3,104 +3,190 @@ ## Getting started - Help us review [open pull requests](https://github.com/neovim/neovim/pulls)! -- Look for [**entry-level**][entry] issues to work on. - - [**documentation**](https://github.com/neovim/neovim/labels/documentation) - improvements are also very helpful. +- Look for [entry-level issues][entry-level] to work on. + - [Documentation](https://github.com/neovim/neovim/labels/documentation) + improvements are also much appreciated. - Look at [Waffle][waffle] to see who is working on what issues. -- Refer to the [the wiki][wiki] for detailed guidance. - -## Issues - -- Search existing issues before raising a new one. -- Include as much detail as possible. In particular, we need to know which - OS you're using. - -## Pull requests - +- If needed, refer to [the wiki][wiki-contributing] for guidance. + +## Reporting problems + +Before reporting an issue: + +- Verify that it hasn't already been reported. +- If not already running the latest version of Neovim, update to it to see if + your problem persists. +- If you're experiencing compile or runtime warnings/failures, try searching for + the error message(s) you received (if any) on [Neovim's issue tracker][github-issues]. + - For installation issues, see [Installing Neovim#troubleshooting][wiki-install-troubleshooting]. + - For build issues, see [Building Neovim#troubleshooting][wiki-building-troubleshooting]. + - For runtime issues: try to reproduce them by running `nvim` with the + smallest possible `nvimrc` (or none at all via `nvim -u NONE`), to rule + out bugs in plugins you're using. If you're using a plugin manager, + comment out your plugins, then add them back in one by one. + +Include as much detail as possible; we generally need to know: + +- What operating system you're using. +- Which version of Neovim you're using. To get this, run `nvim --version` from + a shell, or run `:version` from inside `nvim`. +- Whether the bug is present in Vim (not Neovim), and if so which version of + Vim. It's fine to report Vim bugs on the Neovim bug tracker, but it saves + everyone time if we know from the start that the bug is not a regression + caused by Neovim. +- This isn't required, but what commit introduced the issue for you. You can + use [`git bisect`][git-bisect] for this. + +## Submitting contributions + +- If you're a first-time contributor, please sign the [Neovim Contributor + License Agreement (CLA)][cla] before submitting your pull request. - Make it clear in the issue tracker what you are working on. -- Be descriptive in your PR message: what is it for, why is it needed, etc. -- Don't make cosmetic changes to unrelated files in the same PR. -- If you're a first-time contributor, please sign the - [Neovim Contributor License Agreement (CLA)][cla] before submitting your PR. - -#### Tagging in the issue tracker - -When submitting pull requests, include one of the following tokens in the title: - -* `[WIP]` - Work In Progress. The pull request will change, and there is no need - to review it yet. -* `[RFC]` - Request For Comment. The request needs reviewing and/or comments. -* `[RDY]` - The request is ready to be merged. The request must have been - reviewed by at least one person and have no outstanding issues. -* Default label is assumed to be `[WIP]` if there's no indication otherwise. - -#### Branching & history - -- Use a feature branch, not master. -- Rebase your feature branch onto (upstream) master before raising the PR. -- Keep up to date with changes in (upstream) master so your PR is easy to merge. -- Try to actively tidy your history: combine related commits with interactive - rebasing etc. If your PR is still `[WIP]` don't be afraid to force-push to - your feature branch to tidy your history. - -### For code PRs +- Be descriptive in your pull request description: what is it for, why is it + needed, etc. +- Do ***not*** make cosmetic changes to unrelated files in the same pull + request. This creates noise, making reviews harder to do. If your text + editor strips all trailing whitespace in a file when you edit it, disable + it. + +### Tagging in the issue tracker + +When submitting pull requests (commonly referred to as "PRs"), include one of +the following tags prepended to the title: + +- `[WIP]` - Work In Progress: the PR will change, so while there is no + immediate need for review, the submitter still might appreciate it. +- `[RFC]` - Request For Comment: the PR needs reviewing and/or comments. +- `[RDY]` - Ready: the PR has been reviewed by at least one other person and + has no outstanding issues. + +Assuming the above criteria has been met, feel free to change your PR's tag +yourself, as opposed to waiting for a contributor to do it for you. + +### Branching & history + +- Do ***not*** work on your PR on the master branch, [use a feature branch + instead][git-feature-branch]. +- [Rebase your feature branch onto][git-rebasing] (upstream) master before + opening the PR. +- Keep up to date with changes in (upstream) master so your PR is easy to + merge. +- [Try to actively tidy your history][git-history-rewriting]: combine related + commits with interactive rebasing, separate monolithic commits, etc. If your + PR is still `[WIP]`, feel free to force-push to your feature branch to tidy + your history. + +### For code pull requests #### Testing -- We are unlikely to merge your PR if the Travis build fails. -- The Travis build does not currently run the tests under valgrind, but you - are encouraged to do so locally. +We are unlikely to merge your PR if the Travis build fails: + +- Travis builds are compiled with the [`-Werror`][gcc-warnings] flag, so if + your PR introduces any compiler warnings then the Travis build will fail. +- If any tests fail, the Travis build will fail. See [Building + Neovim#testing][wiki-building-testing] for information on running tests + locally. Tests passing locally doesn't guarantee they'll pass in the Travis + build, as different compilers and platforms will be used. +- Travis runs [Valgrind][valgrind] for the GCC/Linux build, but you may also + do so locally by running the following from a shell: `VALGRIND=1 make test` #### Coding style -We have a [style guide][style] that all new code should follow. However, vast -swathes of the existing vim codebase violate it to some degree, and fixing -them would increase merge conflicts and add noise to `git blame`. Please weigh -those costs when making cosmetic changes. As a rule of thumb, avoid pull -requests dominated by style changes. Feel free to fix up lines that you happen -to be modifying anyway, as long as they look consistent with their -surroundings. Fix anything that looks outright -[barbarous](http://www.orwell.ru/library/essays/politics/english/e_polit) -- -especially if you can't find any editor settings that make it look ok -- but -otherwise err on the side of leaving things as they are. - -For new code, please run [`clint.py`][clint] to detect style errors. It is not -perfect and may have false positives and negatives. To have `clint.py` ignore -certain special cases, put `// NOLINT` at the end of the line. - -We also provide a configuration file for [`clang-format` and -`git-clang-format`][clang-format], which can be used to format code according -to the style guidelines. Be aware this formatting method might need user -supervision. - -#### Commit guidelines - -The purpose of these guidelines is to *make reviews easier* and make the VCS logs more valuable. - -- Try to keep the first line under 70 characters. -- Include further description, if necessary, after a blank line. - - Don't make it too verbose by including obvious things. - - But don't spare clarifications for anything that could be not so obvious. - Some commit messages are pages long, and that's fine if there's no better - place for those comments to live. +We have a [style guide][style-guide] that all new code should follow. +However, large portions of the existing Vim codebase violate it to some +degree, and fixing them would increase merge conflicts and add noise to `git +blame`. + +Weigh those costs when making cosmetic changes. In general, avoid pull +requests dominated by style changes, but feel free to fix up lines that you +happen to be modifying anyway. Fix anything that looks outright +[barbarous](http://www.orwell.ru/library/essays/politics/english/e_polit), but +otherwise prefer to leave things as they are. + +For new code, run `make lint` (which runs [clint.py][clint]) to detect style +errors. Make sure that the file(s) you intend to be linted are not in +`clint-ignored-files.txt`. It's not perfect, so some warnings may be false +positives/negatives. To have `clint.py` ignore certain cases, put `// NOLINT` +at the end of the line. + +We also provide a configuration file for [`clang-format`][clang-format], which +can be used to format code according to the style guidelines. Be aware that +this formatting method might need user supervision. To have `clang-format` +ignore certain line ranges, use the following special comments: + +```c +int formatted_code; +// clang-format off + void unformatted_code ; +// clang-format on + void formatted_code_again; +``` + +### Commit guidelines + +The purpose of these guidelines is to *make reviews easier* and make the +[VCS][vcs] logs more valuable. + +- Try to keep the first line under 72 characters. +- If necessary, Include further description after a blank line. + - Don't make the description too verbose by including obvious things, but + don't spare clarifications for anything that may be not so obvious. + Some commit messages are pages long, and that's fine if there's no + better place for those comments to live. - **Recommended:** Prefix logically-related commits with a consistent - identifier at the beginning of each commit message. + identifier in each commit message. [For example](https://github.com/neovim/neovim/commits?author=elmart), - the following commits are related by task (*Introduce vim namespace*) and + the following commits are related by task (*Introduce nvim namespace*) and scope (*Contrib YCM*). -
`Introduce vim namespace: Contrib YCM: Fix style issues.` -
`Introduce vim namespace: Contrib YCM: Fix build dir calculation` +
`Introduce nvim namespace: Contrib YCM: Fix style issues` +
`Introduce nvim namespace: Contrib YCM: Fix build dir calculation` - Subtasks can be *activity-oriented* (doing different things on the same area) or *scope-oriented* (doing the same thing on different areas). - Granularity helps, but it's conceptual size that matters, not extent size. -- Use the imperative voice: "Fix bug" rather than "Fixed bug" or "Fixes bug." +- Use the [imperative voice][imperative]: "Fix bug" rather than "Fixed bug" or "Fixes bug." + +### Reviewing pull requests + +Using a checklist during reviews is highly recommended, so we [provide one at +the wiki][wiki-review-checklist]. If you think it could be improved, feel free +to edit it. + +Reviewing can be done on GitHub, but you may find it easier to do locally. +Using [`hub`][hub], you can do the following to create a new branch with the +contents of a pull request, such as [#1820][github-pr-1820]: + hub checkout https://github.com/neovim/neovim/pull/1820 + +Use [`git log -p master..FETCH_HEAD`][git-history-filtering] to list all +commits in the feature branch which aren't in the `master` branch; `-p` +shows each commit's diff. To show the whole surrounding function of a change +as context, use the `-W` argument as well. + +You may find it easier to instead use an interactive program for code reviews, +such as [`tig`][tig]. [cla]: https://docs.google.com/forms/d/1u54bpbwzneDIRltFx1TGi2evKxY3w0cOV3vlpj8DPbg/viewform -[clint]: clint.py [clang-format]: http://clang.llvm.org/docs/ClangFormat.html -[entry]: https://github.com/neovim/neovim/issues?labels=entry-level&state=open +[clint]: clint.py +[entry-level]: https://github.com/neovim/neovim/issues?labels=entry-level&state=open +[gcc-warnings]: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html +[git-bisect]: http://git-scm.com/book/tr/v2/Git-Tools-Debugging-with-Git +[git-feature-branch]: https://www.atlassian.com/git/tutorials/comparing-workflows +[git-history-filtering]: https://www.atlassian.com/git/tutorials/git-log/filtering-the-commit-history +[git-history-rewriting]: http://git-scm.com/book/en/v2/Git-Tools-Rewriting-History +[git-rebasing]: http://git-scm.com/book/en/v2/Git-Branching-Rebasing +[github-issues]: https://github.com/neovim/neovim/issues +[github-pr-1820]: https://github.com/neovim/neovim/pull/1820 +[hub]: https://hub.github.com/ [imperative]: http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html -[style]: http://neovim.org/develop/style-guide.xml +[style-guide]: http://neovim.org/develop/style-guide.xml +[tig]: https://github.com/jonas/tig +[valgrind]: http://valgrind.org/ +[vcs]: https://en.wikipedia.org/wiki/Revision_control [waffle]: https://waffle.io/neovim/neovim -[wiki]: https://github.com/neovim/neovim/wiki/Contributing +[wiki-building-testing]: https://github.com/neovim/neovim/wiki/Building-Neovim#testing +[wiki-building-troubleshooting]: https://github.com/neovim/neovim/wiki/Building-Neovim#troubleshootingfaq +[wiki-contributing]: https://github.com/neovim/neovim/wiki/Contributing +[wiki-install-troubleshooting]: https://github.com/neovim/neovim/wiki/Installing-Neovim#troubleshooting +[wiki-review-checklist]: https://github.com/neovim/neovim/wiki/Code-review-checklist -- cgit From 82d2b8a45fabdd3e28792a1209bdf9ea9cfbc237 Mon Sep 17 00:00:00 2001 From: Michael Reed Date: Sat, 28 Mar 2015 13:00:51 -0400 Subject: CONTRIBUTING.md: Review Helped-by: Florian Walch Helped-by: oni-link --- CONTRIBUTING.md | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5133f50ad0..2cb202b201 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -129,20 +129,21 @@ The purpose of these guidelines is to *make reviews easier* and make the [VCS][vcs] logs more valuable. - Try to keep the first line under 72 characters. -- If necessary, Include further description after a blank line. +- If necessary, include further description after a blank line. - Don't make the description too verbose by including obvious things, but don't spare clarifications for anything that may be not so obvious. Some commit messages are pages long, and that's fine if there's no better place for those comments to live. - **Recommended:** Prefix logically-related commits with a consistent - identifier in each commit message. + identifier in each commit message. For already used identifiers, see the + commit history for the respective file(s) you're editing. [For example](https://github.com/neovim/neovim/commits?author=elmart), the following commits are related by task (*Introduce nvim namespace*) and - scope (*Contrib YCM*). + sub-task (*Contrib YCM*).
`Introduce nvim namespace: Contrib YCM: Fix style issues`
`Introduce nvim namespace: Contrib YCM: Fix build dir calculation` - - Subtasks can be *activity-oriented* (doing different things on the same area) - or *scope-oriented* (doing the same thing on different areas). + - Sub-tasks can be *activity-oriented* (doing different things on the same area) + or *scope-oriented* (doing the same thing in different areas). - Granularity helps, but it's conceptual size that matters, not extent size. - Use the [imperative voice][imperative]: "Fix bug" rather than "Fixed bug" or "Fixes bug." @@ -186,7 +187,7 @@ such as [`tig`][tig]. [vcs]: https://en.wikipedia.org/wiki/Revision_control [waffle]: https://waffle.io/neovim/neovim [wiki-building-testing]: https://github.com/neovim/neovim/wiki/Building-Neovim#testing -[wiki-building-troubleshooting]: https://github.com/neovim/neovim/wiki/Building-Neovim#troubleshootingfaq +[wiki-building-troubleshooting]: https://github.com/neovim/neovim/wiki/Building-Neovim#troubleshooting [wiki-contributing]: https://github.com/neovim/neovim/wiki/Contributing [wiki-install-troubleshooting]: https://github.com/neovim/neovim/wiki/Installing-Neovim#troubleshooting [wiki-review-checklist]: https://github.com/neovim/neovim/wiki/Code-review-checklist -- cgit