mirror of
https://github.com/nasa/openmct.git
synced 2024-12-18 20:57:53 +00:00
299 lines
12 KiB
Markdown
299 lines
12 KiB
Markdown
|
# Contributing to Open MCT Web
|
|||
|
|
|||
|
This document describes the process of contributing to Open MCT Web as well
|
|||
|
as the standards that will be applied when evaluating contributions.
|
|||
|
|
|||
|
Please be aware that additional agreements will be necessary before we can
|
|||
|
accept changes from external contributors.
|
|||
|
|
|||
|
## Summary
|
|||
|
|
|||
|
The short version:
|
|||
|
|
|||
|
1. Write your contribution.
|
|||
|
2. Make sure your contribution meets code, test, and commit message
|
|||
|
standards as described below.
|
|||
|
3. Submit a pull request from a topic branch back to `master`. Include a check
|
|||
|
list, as described below. (Optionally, assign this to a specific member
|
|||
|
for review.)
|
|||
|
4. Respond to any discussion. When the reviewer decides it's ready, they
|
|||
|
will merge back `master` and fill out their own check list.
|
|||
|
|
|||
|
## Contribution Process
|
|||
|
|
|||
|
Open MCT Web uses git for software version control, and for branching and
|
|||
|
merging. The central repository is at
|
|||
|
https://github.com/nasa/openmctweb.git.
|
|||
|
|
|||
|
### Roles
|
|||
|
|
|||
|
References to roles are made throughout this document. These are not intended
|
|||
|
to reflect titles or long-term job assignments; rather, these are used as
|
|||
|
descriptors to refer to members of the development team performing tasks in
|
|||
|
the check-in process. These roles are:
|
|||
|
|
|||
|
* _Author_: The individual who has made changes to files in the software
|
|||
|
repository, and wishes to check these in.
|
|||
|
* _Reviewer_: The individual who reviews changes to files before they are
|
|||
|
checked in.
|
|||
|
* _Integrator_: The individual who performs the task of merging these files.
|
|||
|
Usually the reviewer.
|
|||
|
|
|||
|
### Branching
|
|||
|
|
|||
|
Three basic types of branches may be included in the above repository:
|
|||
|
|
|||
|
1. Master branch.
|
|||
|
2. Topic branches.
|
|||
|
3. Developer branches.
|
|||
|
|
|||
|
Branches which do not fit into the above categories may be created and used
|
|||
|
during the course of development for various reasons, such as large-scale
|
|||
|
refactoring of code or implementation of complex features which may cause
|
|||
|
instability. In these exceptional cases it is the responsibility of the
|
|||
|
developer who initiates the task which motivated this branching to
|
|||
|
communicate to the team the role of these branches and any associated
|
|||
|
procedures for the duration of their use.
|
|||
|
|
|||
|
#### Master Branch
|
|||
|
|
|||
|
The role of the `master` branches is to represent the latest
|
|||
|
"ready for test" version of the software. Source code on the master
|
|||
|
branch has undergone peer review, and will undergo regular automated
|
|||
|
testing with notification on failure. Master branches may be unstable
|
|||
|
(particularly for recent features), but the intent is for the stability of
|
|||
|
any features on master branches to be non-decreasing. It is the shared
|
|||
|
responsibility of authors, reviewers, and integrators to ensure this.
|
|||
|
|
|||
|
#### Topic Branches
|
|||
|
|
|||
|
Topic branches are used by developers to perform and record work on issues.
|
|||
|
|
|||
|
Topic branches need not necessarily be stable, even when pushed to the
|
|||
|
central repository; in fact, the practice of making incremental commits
|
|||
|
while working on an issue and pushing these to the central repository is
|
|||
|
encouraged, to avoid lost work and to share work-in-progress. (Small commits
|
|||
|
also help isolate changes, which can help in identifying which change
|
|||
|
introduced a defect, particularly when that defect went unnoticed for some
|
|||
|
time, e.g. using `git bisect`.)
|
|||
|
|
|||
|
Topic branches should be named according to their corresponding issue
|
|||
|
identifiers, all lower case, without hyphens. (e.g. branch mct9 would refer
|
|||
|
to issue #9.)
|
|||
|
|
|||
|
In some cases, work on an issue may warrant the use of multiple divergent
|
|||
|
branches; for instance, when a developer wants to try more than one solution
|
|||
|
and compare them, or when a "dead end" is reached and an initial approach to
|
|||
|
resolving an issue needs to be abandoned. In these cases, a short suffix
|
|||
|
should be added to the additional branches; this may be simply a single
|
|||
|
character (e.g. wtd481b) or, where useful, a descriptive term for what
|
|||
|
distinguishes the branches (e.g. wtd481verbose). It is the responsibility of
|
|||
|
the author to communicate which branch is intended to be merged to both the
|
|||
|
reviewer and the integrator.
|
|||
|
|
|||
|
#### Developer Branches
|
|||
|
|
|||
|
Developer branches are any branches used for purposes outside of the scope
|
|||
|
of the above; e.g. to try things out, or maintain a "my latest stuff"
|
|||
|
branch that is not delayed by the review and integration process. These
|
|||
|
may be pushed to the central repository, and may follow any naming convention
|
|||
|
desired so long as the owner of the branch is identifiable, and so long as
|
|||
|
the name chosen could not be mistaken for a topic or master branch.
|
|||
|
|
|||
|
### Merging
|
|||
|
|
|||
|
When development is complete on an issue, the first step toward merging it
|
|||
|
back into the master branch is to file a Pull Request. The contributions
|
|||
|
should meet code, test, and commit message standards as described below,
|
|||
|
and the pull request should include a completed author checklist, also
|
|||
|
as described below. Pull requests may be assigned to specific team
|
|||
|
members when appropriate (e.g. to draw to a specific person's attention.)
|
|||
|
|
|||
|
Code review should take place using discussion features within the pull
|
|||
|
request. When the reviewer is satisfied, they should add a comment to
|
|||
|
the pull request containing the reviewer checklist (from below) and complete
|
|||
|
the merge back to the master branch.
|
|||
|
|
|||
|
## Standards
|
|||
|
|
|||
|
Contributions to Open MCT Web are expected to meet the following standards.
|
|||
|
In addition, reviewers should use general discretion before accepting
|
|||
|
changes.
|
|||
|
|
|||
|
### Code Standards
|
|||
|
|
|||
|
JavaScript sources in Open MCT Web must satisfy JSLint under its default
|
|||
|
settings. This is verified by the command line build.
|
|||
|
|
|||
|
#### Code Guidelines
|
|||
|
|
|||
|
JavaScript sources in Open MCT Web should:
|
|||
|
|
|||
|
* Use four spaces for indentation. Tabs should not be used.
|
|||
|
* Include JSDoc for any exposed API (e.g. public methods, constructors.)
|
|||
|
* Include non-JSDoc comments as-needed for explaining private variables,
|
|||
|
methods, or algorithms when they are non-obvious.
|
|||
|
* Define one public class per script, expressed as a constructor function
|
|||
|
returned from an AMD-style module.
|
|||
|
* Follow “Java-like” naming conventions. These includes:
|
|||
|
* Classes should use camel case, first letter capitalized
|
|||
|
(e.g. SomeClassName.)
|
|||
|
* Methods, variables, fields, and function names should use camel case,
|
|||
|
first letter lower-case (e.g. someVariableName.) Constants
|
|||
|
(variables or fields which are meant to be declared and initialized
|
|||
|
statically, and never changed) should use only capital letters, with
|
|||
|
underscores between words (e.g. SOME_CONSTANT.)
|
|||
|
* File name should be the name of the exported class, plus a .js extension
|
|||
|
(e.g. SomeClassName.js)
|
|||
|
* Avoid anonymous functions, except when functions are short (a few lines)
|
|||
|
and/or their inclusion makes sense within the flow of the code
|
|||
|
(e.g. as arguments to a forEach call.)
|
|||
|
* Avoid deep nesting (especially of functions), except where necessary
|
|||
|
(e.g. due to closure scope.)
|
|||
|
* End with a single new-line character.
|
|||
|
* Expose public methods by declaring them on the class's prototype.
|
|||
|
* Within a given function's scope, do not mix declarations and imperative
|
|||
|
code, and present these in the following order:
|
|||
|
* First, variable declarations and initialization.
|
|||
|
* Second, function declarations.
|
|||
|
* Third, imperative statements.
|
|||
|
* Finally, the returned value.
|
|||
|
|
|||
|
Deviations from Open MCT Web code style guidelines require two-party agreement,
|
|||
|
typically from the author of the change and its reviewer.
|
|||
|
|
|||
|
#### Code Example
|
|||
|
|
|||
|
```js
|
|||
|
/*global define*/
|
|||
|
|
|||
|
/**
|
|||
|
* Bundles should declare themselves as namespaces in whichever source
|
|||
|
* file is most like the "main point of entry" to the bundle.
|
|||
|
* @namespace some/bundle
|
|||
|
*/
|
|||
|
define(
|
|||
|
['./OtherClass'],
|
|||
|
function (OtherClass) {
|
|||
|
"use strict";
|
|||
|
|
|||
|
/**
|
|||
|
* A summary of how to use this class goes here.
|
|||
|
*
|
|||
|
* @constructor
|
|||
|
* @memberof some/bundle
|
|||
|
*/
|
|||
|
function ExampleClass() {
|
|||
|
}
|
|||
|
|
|||
|
// Methods which are not intended for external use should
|
|||
|
// not have JSDoc (or should be marked @private)
|
|||
|
ExampleClass.prototype.privateMethod = function () {
|
|||
|
};
|
|||
|
|
|||
|
/**
|
|||
|
* A summary of this method goes here.
|
|||
|
* @param {number} n a parameter
|
|||
|
* @returns {number} a return value
|
|||
|
*/
|
|||
|
ExampleClass.prototype.publicMethod = function (n) {
|
|||
|
return n * 2;
|
|||
|
}
|
|||
|
|
|||
|
return ExampleClass;
|
|||
|
}
|
|||
|
);
|
|||
|
```
|
|||
|
|
|||
|
### Test Standards
|
|||
|
|
|||
|
Automated testing shall occur whenever changes are merged into the main
|
|||
|
development branch and must be confirmed alongside any pull request.
|
|||
|
|
|||
|
Automated tests are typically unit tests which exercise individual software
|
|||
|
components. Tests are subject to code review along with the actual
|
|||
|
implementation, to ensure that tests are applicable and useful.
|
|||
|
|
|||
|
Examples of useful tests:
|
|||
|
* Tests which replicate bugs (or their root causes) to verify their
|
|||
|
resolution.
|
|||
|
* Tests which reflect details from software specifications.
|
|||
|
* Tests which exercise edge or corner cases among inputs.
|
|||
|
* Tests which verify expected interactions with other components in the
|
|||
|
system.
|
|||
|
|
|||
|
During automated testing, code coverage metrics will be reported. Line
|
|||
|
coverage must remain at or above 80%.
|
|||
|
|
|||
|
### Commit Message Standards
|
|||
|
|
|||
|
Commit messages should:
|
|||
|
|
|||
|
* Contain a one-line subject, followed by one line of white space,
|
|||
|
followed by one or more descriptive paragraphs, each separated by one
|
|||
|
 line of white space.
|
|||
|
* Contain a short (usually one word) reference to the feature or subsystem
|
|||
|
the commit effects, in square brackets, at the start of the subject line
|
|||
|
(e.g. `[Documentation] Draft of check-in process`)
|
|||
|
* Contain a reference to a relevant issue number in the body of the commit.
|
|||
|
* This is important for traceability; while branch names also provide this,
|
|||
|
you cannot tell from looking at a commit what branch it was authored on.
|
|||
|
* Describe the change that was made, and any useful rationale therefore.
|
|||
|
* Comments in code should explain what things do, commit messages describe
|
|||
|
how they came to be done that way.
|
|||
|
* Provide sufficient information for a reviewer to understand the changes
|
|||
|
made and their relationship to previous code.
|
|||
|
|
|||
|
Commit messages should not:
|
|||
|
|
|||
|
* Exceed 54 characters in length on the subject line.
|
|||
|
* Exceed 72 characters in length in the body of the commit.
|
|||
|
* Except where necessary to maintain the structure of machine-readable or
|
|||
|
machine-generated text (e.g. error messages)
|
|||
|
|
|||
|
See [Contributing to a Project](http://git-scm.com/book/ch5-2.html) from
|
|||
|
Pro Git by Shawn Chacon and Ben Straub for a bit of the rationale behind
|
|||
|
these standards.
|
|||
|
|
|||
|
## Issue Reporting
|
|||
|
|
|||
|
Issues are tracked at https://github.com/nasa/openmctweb/issues
|
|||
|
|
|||
|
Issues should include:
|
|||
|
|
|||
|
* A short description of the issue encountered.
|
|||
|
* A longer-form description of the issue encountered. When possible, steps to
|
|||
|
reproduce the issue.
|
|||
|
* When possible, a description of the impact of the issue. What use case does
|
|||
|
it impede?
|
|||
|
* An assessment of the severity of the issue.
|
|||
|
|
|||
|
Issue severity is categorized as follows (in ascending order):
|
|||
|
|
|||
|
* _Trivial_: Minimal impact on the usefulness and functionality of the
|
|||
|
software; a "nice-to-have."
|
|||
|
* _(Unspecified)_: Major loss of functionality or impairment of use.
|
|||
|
* _Critical_: Large-scale loss of functionality or impairment of use,
|
|||
|
such that remaining utility becomes marginal.
|
|||
|
* _Blocker_: Harmful or otherwise unacceptable behavior. Must fix.
|
|||
|
|
|||
|
## Check Lists
|
|||
|
|
|||
|
The following check lists should be completed and attached to pull requests
|
|||
|
when they are filed (author checklist) and when they are merged (reviewer
|
|||
|
checklist.)
|
|||
|
|
|||
|
### Author Checklist
|
|||
|
|
|||
|
1. Changes address original issue?
|
|||
|
2. Unit tests included and/or updated with changes?
|
|||
|
3. Command line build passes?
|
|||
|
4. Expect to pass code review?
|
|||
|
|
|||
|
### Reviewer Checklist
|
|||
|
|
|||
|
1. Changes appear to address issue?
|
|||
|
2. Appropriate unit tests included?
|
|||
|
3. Code style and in-line documentation are appropriate?
|
|||
|
4. Commit messages meet standards?
|