From 533dfc42a854f0a5992efb1e6a91ed7fed8d8520 Mon Sep 17 00:00:00 2001 From: shelley vohr Date: Mon, 12 Feb 2018 09:09:38 -0500 Subject: [PATCH] enhance documentation around contributing to electron (#11887) * add issues document * add documentation coding style to doc * copyediting * replace `nodejs/node` with `electron/electron` * fix commasplice * fix two most important... s/is/are/ * omit unnecessary words * add pull requests doc * copyediting * add general code style to styleguide * updates to CONTRIBUTING.md * copyediting * mark shell blocks as ```sh * mitigate phrase duplication e.g. 'best practice' * lots of opinionated changes to omit unnecessary words * fix numbering & re-apply changes that I overwrote --- CONTRIBUTING.md | 91 ++++-------- docs/development/coding-style.md | 24 +++ docs/development/issues.md | 109 ++++++++++++++ docs/development/pull-requests.md | 235 ++++++++++++++++++++++++++++++ 4 files changed, 397 insertions(+), 62 deletions(-) create mode 100644 docs/development/issues.md create mode 100644 docs/development/pull-requests.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b4d585517292..5d04da05397c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -10,21 +10,15 @@ The following is a set of guidelines for contributing to Electron. These are just guidelines, not rules, use your best judgment and feel free to propose changes to this document in a pull request. -## Submitting Issues +## [Issues](https://electronjs.org/docs/development/issues) -### Creating Issues -* You can create an issue [here](https://github.com/electron/electron/issues/new), -but before doing that please read the notes below and include as many details as -possible with your report. If you can, please include: - * The version of Electron you are using - * The operating system you are using - * If applicable, what you were doing when the issue arose and what you - expected to happen -* Other things that will help resolve your issue: - * Screenshots and animated GIFs - * Error output that appears in your terminal, dev tools or as an alert - * Perform a [cursory search](https://github.com/electron/electron/issues?utf8=✓&q=is%3Aissue+) - to see if a similar issue has already been submitted +Issues are created [here](https://github.com/electron/electron/issues/new). + +* [How to Contribute in Issues](https://electronjs.org/docs/development/issues#how-to-contribute-in-issues) +* [Asking for General Help](https://electronjs.org/docs/development/issues#asking-for-general-help) +* [Submitting a Bug Report](https://electronjs.org/docs/development/issues#submitting-a-bug-report) +* [Triaging a Bug Report](https://electronjs.org/docs/development/issues#triaging-a-bug-report) +* [Resolving a Bug Report](https://electronjs.org/docs/development/issues#resolving-a-bug-report) ### Issue Maintenance and Closure * If an issue is inactive for 45 days (no activity of any kind), it will be @@ -34,56 +28,29 @@ the issue will be closed. * If an issue has been closed and you still feel it's relevant, feel free to ping a maintainer or add a comment! -## Development -* Build instructions can be found in [docs/development](docs/development). +## [Pull Requests](https://electronjs.org/docs/development/pull-requests) -## Submitting Pull Requests +Pull Requests are the way concrete changes are made to the code, documentation, +dependencies, and tools contained in the `electron/electron` repository. -* Include screenshots and animated GIFs in your pull request whenever possible. -* Follow the JavaScript, C++, and Python [coding style defined in docs](/docs/development/coding-style.md). -* Write documentation in [Markdown](https://daringfireball.net/projects/markdown). - See the [Documentation Styleguide](/docs/styleguide.md). -* Use short, present tense commit messages. See [Commit Message Styleguide](#git-commit-messages). +* [Setting up your local environment](https://electronjs.org/docs/development/pull-requests#setting-up-your-local-environment) + * [Step 1: Fork](https://electronjs.org/docs/development/pull-requests#step-1-fork) + * [Step 2: Build](https://electronjs.org/docs/development/pull-requests#step-2-build) + * [Step 3: Branch](https://electronjs.org/docs/development/pull-requests#step-3-branch) +* [The Process of Making Changes](https://electronjs.org/docs/development/pull-requests#the-process-of-making-changes) + * [Step 4: Code](https://electronjs.org/docs/development/pull-requests#step-4-code) + * [Step 5: Commit](https://electronjs.org/docs/development/pull-requests#step-5-commit) + * [Commit message guidelines](https://electronjs.org/docs/development/pull-requests#commit-message-guidelines) + * [Step 6: Rebase](https://electronjs.org/docs/development/pull-requests#step-6-rebase) + * [Step 7: Test](https://electronjs.org/docs/development/pull-requests#step-7-test) + * [Step 8: Push](https://electronjs.org/docs/development/pull-requests#step-8-push) + * [Step 8: Opening the Pull Request](https://electronjs.org/docs/development/pull-requests#step-8-opening-the-pull-request) + * [Step 9: Discuss and Update](#step-9-discuss-and-update) + * [Approval and Request Changes Workflow](https://electronjs.org/docs/development/pull-requests#approval-and-request-changes-workflow) + * [Step 10: Landing](https://electronjs.org/docs/development/pull-requests#step-10-landing) + * [Continuous Integration Testing](https://electronjs.org/docs/development/pull-requests#continuous-integration-testing) -## Styleguides +## Style Guides -### General Code +See [Coding Style](https://electronjs.org/docs/development/coding-style) for information about which standards Electron adheres to in different parts of its codebase. -* End files with a newline. -* Place requires in the following order: - * Built in Node Modules (such as `path`) - * Built in Electron Modules (such as `ipc`, `app`) - * Local Modules (using relative paths) -* Place class properties in the following order: - * Class methods and properties (methods starting with a `@`) - * Instance methods and properties -* Avoid platform-dependent code: - * Use `path.join()` to concatenate filenames. - * Use `os.tmpdir()` rather than `/tmp` when you need to reference the - temporary directory. -* Using a plain `return` when returning explicitly at the end of a function. - * Not `return null`, `return undefined`, `null`, or `undefined` - -### Git Commit Messages - -* Use the present tense ("Add feature" not "Added feature") -* Use the imperative mood ("Move cursor to..." not "Moves cursor to...") -* Limit the first line to 72 characters or less -* Reference issues and pull requests liberally -* When only changing documentation, include `[ci skip]` in the commit description -* Consider starting the commit message with an applicable emoji: - * :art: `:art:` when improving the format/structure of the code - * :racehorse: `:racehorse:` when improving performance - * :non-potable_water: `:non-potable_water:` when plugging memory leaks - * :memo: `:memo:` when writing docs - * :penguin: `:penguin:` when fixing something on Linux - * :apple: `:apple:` when fixing something on macOS - * :checkered_flag: `:checkered_flag:` when fixing something on Windows - * :bug: `:bug:` when fixing a bug - * :fire: `:fire:` when removing code or files - * :green_heart: `:green_heart:` when fixing the CI build - * :white_check_mark: `:white_check_mark:` when adding tests - * :lock: `:lock:` when dealing with security - * :arrow_up: `:arrow_up:` when upgrading dependencies - * :arrow_down: `:arrow_down:` when downgrading dependencies - * :shirt: `:shirt:` when removing linter warnings diff --git a/docs/development/coding-style.md b/docs/development/coding-style.md index ffaa9db08fc0..cf11a46e4500 100644 --- a/docs/development/coding-style.md +++ b/docs/development/coding-style.md @@ -5,6 +5,23 @@ These are the style guidelines for coding in Electron. You can run `npm run lint` to show any style issues detected by `cpplint` and `eslint`. +## General Code + +* End files with a newline. +* Place requires in the following order: + * Built in Node Modules (such as `path`) + * Built in Electron Modules (such as `ipc`, `app`) + * Local Modules (using relative paths) +* Place class properties in the following order: + * Class methods and properties (methods starting with a `@`) + * Instance methods and properties +* Avoid platform-dependent code: + * Use `path.join()` to concatenate filenames. + * Use `os.tmpdir()` rather than `/tmp` when you need to reference the + temporary directory. +* Using a plain `return` when returning explicitly at the end of a function. + * Not `return null`, `return undefined`, `null`, or `undefined` + ## C++ and Python For C++ and Python, we follow Chromium's [Coding @@ -21,6 +38,13 @@ document. The document mentions some special types, scoped types (that automatically release their memory when going out of scope), logging mechanisms etc. +## Documentation + +* Write [remark](https://github.com/remarkjs/remark) markdown style + +You can run `npm run lint-docs` to ensure that your documentation changes are +formatted correctly. + ## JavaScript * Write [standard](https://npm.im/standard) JavaScript style. diff --git a/docs/development/issues.md b/docs/development/issues.md new file mode 100644 index 000000000000..4bb4fe357d5f --- /dev/null +++ b/docs/development/issues.md @@ -0,0 +1,109 @@ +# Issues In Electron + +# Issues + +* [How to Contribute in Issues](#how-to-contribute-in-issues) +* [Asking for General Help](#asking-for-general-help) +* [Submitting a Bug Report](#submitting-a-bug-report) +* [Triaging a Bug Report](#triaging-a-bug-report) +* [Resolving a Bug Report](#resolving-a-bug-report) + +## How to Contribute in Issues + +For any issue, there are fundamentally three ways an individual can +contribute: + +1. By opening the issue for discussion: If you believe that you have found + a new bug in Electron, you should report it by creating a new issue in + the `electron/electron` issue tracker. +2. By helping to triage the issue: You can do this either by providing + assistive details (a reproducible test case that demonstrates a bug) or by + providing suggestions to address the issue. +3. By helping to resolve the issue: This can be done by demonstrating + that the issue is not a bug or is fixed; but more often, by opening + a pull request that changes the source in `electron/electron` in a + concrete and reviewable manner. + +## Asking for General Help + +Because the level of activity in the `electron/electron` repository is +so high, questions or requests for general help using Electron should +be directed at the [community slack channel](https://atomio.slack.com) +or the [forum](https://discuss.atom.io/c/electron). + +## Submitting a Bug Report + +When opening a new issue in the `electron/electron` issue tracker, users +will be presented with a template that should be filled in. + +```markdown + + +* Electron version: +* Operating system: + +### Expected behavior + + + +### Actual behavior + + + +### How to reproduce + + +``` + +If you believe that you have found a bug in Electron, please fill out this +form to the best of your ability. + +The two most important pieces of information needed to evaluate the report are +a description of the bug and a simple test case to recreate it. It easier to fix +a bug if it can be reproduced. + +See [How to create a Minimal, Complete, and Verifiable example](https://stackoverflow.com/help/mcve). + +## Triaging a Bug Report + +It's common for open issues to involve discussion. Some contributors may +have differing opinions, including whether the behavior is a bug or feature. +This discussion is part of the process and should be kept focused, helpful, +and professional. + +Terse responses that provide neither additional context nor supporting detail +are not helpful or professional. To many, such responses are annoying and +unfriendly. + +Contributors are encouraged to solve issues collaboratively and help one +another make progress. If encounter an issue that you feel is invalid, or +which contains incorrect information, explain *why* you feel that way with +additional supporting context, and be willing to be convinced that you may +be wrong. By doing so, we can often reach the correct outcome faster. + +## Resolving a Bug Report + +Most issues are resolved by opening a pull request. The process for opening and +reviewing a pull request is similar to that of opening and triaging issues, but +carries with it a necessary review and approval workflow that ensures that the +proposed changes meet the minimal quality and functional guidelines of the +Electron project. diff --git a/docs/development/pull-requests.md b/docs/development/pull-requests.md new file mode 100644 index 000000000000..83302575fa43 --- /dev/null +++ b/docs/development/pull-requests.md @@ -0,0 +1,235 @@ +# Pull Requests + +* [Dependencies](#dependencies) +* [Setting up your local environment](#setting-up-your-local-environment) + * [Step 1: Fork](#step-1-fork) + * [Step 2: Build](#step-2-build) + * [Step 3: Branch](#step-3-branch) +* [Making Changes](#making-changes) + * [Step 4: Code](#step-4-code) + * [Step 5: Commit](#step-5-commit) + * [Commit message guidelines](#commit-message-guidelines) + * [Step 6: Rebase](#step-6-rebase) + * [Step 7: Test](#step-7-test) + * [Step 8: Push](#step-8-push) + * [Step 9: Opening the Pull Request](#step-8-opening-the-pull-request) + * [Step 10: Discuss and Update](#step-9-discuss-and-update) + * [Approval and Request Changes Workflow](#approval-and-request-changes-workflow) + * [Step 11: Landing](#step-10-landing) + * [Continuous Integration Testing](#continuous-integration-testing) + +## Setting up your local environment + +### Step 1: Fork + +Fork the project [on GitHub](https://github.com/electron/electron) and clone your fork +locally. + +```sh +$ git clone git@github.com:username/electron.git +$ cd electron +$ git remote add upstream https://github.com/electron/electron.git +$ git fetch upstream +``` + +### Step 2: Build + +Build steps and dependencies differ slightly depending on your operating system. +See these detailed guides on building Electron locally: +* [Building on MacOS](https://electronjs.org/docs/development/build-instructions-osx) +* [Building on Linux](https://electronjs.org/docs/development/build-instructions-linux) +* [Building on Windows](https://electronjs.org/docs/development/build-instructions-windows) + +Once you've built the project locally, you're ready to start making changes! + +### Step 3: Branch + +To keep your development environment organized, create local branches to +hold your work. These should be branched directly off of the `master` branch. + +```sh +$ git checkout -b my-branch -t upstream/master +``` + +## Making Changes + +### Step 4: Code + +Most pull requests opened against the `electron/electron` repository include +changes to either the C/C++ code in the `atom/` or `brightray/` folders, +the JavaScript code in the `lib/` folder, the documentation in `docs/api/` +or tests in the `spec/` folder. + +Please be sure to run `npm run lint` from time to time on any code changes +to ensure that they follow the project's code style. + +See [coding style](https://electronjs.org/docs/development/coding-style) for +more information about best practice when modifying code in different parts of +the project. + +### Step 5: Commit + +It is recommended to keep your changes grouped logically within individual +commits. Many contributors find it easier to review changes that are split +across multiple commits. There is no limit to the number of commits in a +pull request. + +```sh +$ git add my/changed/files +$ git commit +``` + +Note that multiple commits often get squashed when they are landed. + +#### Commit message guidelines + +A good commit message should describe what changed and why. + +1. The first line should: + - contain a short description of the change (preferably 50 characters or less, + and no more than 72 characters) + - be entirely in lowercase with the exception of proper nouns, acronyms, and + the words that refer to code, like function/variable names + + Examples: + - `updated osx build documentation for new sdk` + - `fixed typos in atom_api_menu.h` + + +2. Keep the second line blank. +3. Wrap all other lines at 72 columns. + +See [this article](https://chris.beams.io/posts/git-commit/) for more examples +of how to write good git commit messages. + +### Step 6: Rebase + +Once you have committed your changes, it is a good idea to use `git rebase` +(not `git merge`) to synchronize your work with the main repository. + +```sh +$ git fetch upstream +$ git rebase upstream/master +``` + +This ensures that your working branch has the latest changes from `electron/electron` +master. + +### Step 7: Test + +Bug fixes and features should always come with tests. A +[testing guide](https://electronjs.org/docs/development/testing) has been +provided to make the process easier. Looking at other tests to see how they +should be structured can also help. + +Before submitting your changes in a pull request, always run the full +test suite. To run the tests: + +```sh +$ npm run test +``` + +Make sure the linter does not report any issues and that all tests pass. +Please do not submit patches that fail either check. + +If you are updating tests and just want to run a single spec to check it: + +```sh +$ npm run test -match=menu +``` + +The above would only run spec modules matching `menu`, which is useful for +anyone who's working on tests that would otherwise be at the very end of +the testing cycle. + +### Step 8: Push + +Once your commits are ready to go -- with passing tests and linting -- +begin the process of opening a pull request by pushing your working branch +to your fork on GitHub. + +```sh +$ git push origin my-branch +``` + +### Step 9: Opening the Pull Request + +From within GitHub, opening a new pull request will present you with a template +that should be filled out: + +```markdown + +``` + +### Step 10: Discuss and update + +You will probably get feedback or requests for changes to your pull request. +This is a big part of the submission process so don't be discouraged! Some +contributors may sign off on the pull request right away. Others may have +detailed comments or feedback. This is a necessary part of the process +in order to evaluate whether the changes are correct and necessary. + +To make changes to an existing pull request, make the changes to your local +branch, add a new commit with those changes, and push those to your fork. +GitHub will automatically update the pull request. + +```sh +$ git add my/changed/files +$ git commit +$ git push origin my-branch +``` + +There are a number of more advanced mechanisms for managing commits using +`git rebase` that can be used, but are beyond the scope of this guide. + +Feel free to post a comment in the pull request to ping reviewers if you are +awaiting an answer on something. If you encounter words or acronyms that +seem unfamiliar, refer to this +[glossary](https://sites.google.com/a/chromium.org/dev/glossary). + +#### Approval and Request Changes Workflow + +All pull requests require approval from a [Code Owner](https://github.com/orgs/electron/teams/code-owners) of the area you +modified in order to land. Whenever a maintainer reviews a pull request they +may request changes. These may be small, such as fixing a typo, or may involve +substantive changes. Such requests are intended to be helpful, but at times +may come across as abrupt or unhelpful, especially if they do not include +concrete suggestions on *how* to change them. + +Try not to be discouraged. If you feel that a review is unfair, say so or seek +the input of another project contributor. Often such comments are the result of +a reviewer having taken insufficient time to review and are not ill-intended. +Such difficulties can often be resolved with a bit of patience. That said, +reviewers should be expected to provide helpful feeback. + +### Step 11: Landing + +In order to land, a pull request needs to be reviewed and approved by +at least one Electron Code Owner and pass CI. After that, if there are no +objections from other contributors, the pull request can be merged. + +Congratulations and thanks for your contribution! + +### Continuous Integration Testing + +Every pull request is tested on the Continuous Integration (CI) system to +confirm that it works on Electron's supported platforms. + +Ideally, the pull request will pass ("be green") on all of CI's platforms. +This means that all tests pass and there are no linting errors. However, +it is not uncommon for the CI infrastructure itself to fail on specific +platforms or for so-called "flaky" tests to fail ("be red"). Each CI +failure must be manually inspected to determine the cause. + +CI starts automatically when you open a pull request, but only +[Releasers](https://github.com/orgs/electron/teams/releasers/members) +can restart a CI run. If you believe CI is giving a false negative, +ask a Releaser to restart the tests. +