Code Contribution Guidelines

Tyler Keating edited this page Jul 24, 2013 · 11 revisions

Code Contribution Guidelines

The following policy describes the standards that should be met for all code acceptance. Adherence to the policy is strongly recommended, since the future costs of repairing sloppy work are greater than the upfront costs of preventing it.

When submitting a pull request, please ensure that the following criteria have been met:


1. Does the code meet style and readability standards? Are the new functions properly documented with JsDoc?

All code should follow a clean and well thought out style and include proper JsDoc tags (see the Documentation Guide). There is an easy way to verify your code using JSHint, which will check your code for real errors, possible errors as well as style problems. For simplicity sake, all code should be formatted according to JSHint's recommendations using the following three .jshintrc options: indent: 2, trailing: true, white: true.

2. Is the new code properly documented inline?

Writing comments that will assist another person to quickly understand what is happening within a function is encouraged. This does not need to become tedious, just to highlight sections within the function. Grouping code into describable sections is also a good style habit.

3. Are the new functions sufficiently unit tested?

Having several tests that exercise each function under various circumstances is best. Attempting to misuse your code through tests is the best way to discover and prevent edge cases.

4. Do all the unit tests pass?

The Reviewer will run all of the unit tests in the project and they should all continue to pass, you can do the same pre-submission. There is a guide that describes this process here: Running Unit Tests.


5A. Are all the removed functions and classes sufficiently deprecated?

Deprecation should include adding a deprecation warning when the function is called or in the case of a deprecated Class, when the Class is instantiated. It should also include a note of the version that the deprecation occurred in so that the deprecated code can be removed in the future. Finally, if possible it should include full backwards compatibility or else clearly describe the steps to migrate from using the previous functions and Classes to using the new functions and Classes. This description can appear in the deprecation warning.


5B. Have unit tests been included that reveal the bug?

Each bug fix must include unit tests that fail without the fix and pass with the fix. The reason is simple, a bug can re-appear in the future, but the likelihood of this is reduced if there is now a unit test that will flag this. As well, the failing unit test is the easiest way for a Reviewer to understand the bug and therefore understand the proposed fix.

NOTE: Only new code that applies cleanly against the master branch will be accepted. An exception to this rule is that bug fixes against the previous minor release will also be accepted.

For example, if the release history was 1.9.1 (patch), 1.9.0 (minor), 1.8.3 (patch), 1.8.2 (patch), 1.8.1 (patch), 1.8.0 (minor), 1.6.0 (minor), ... then code intended for version 1.9.3 and bug fixes intended for version 1.8.4 would be accepted.

The reason for this policy is that it is already a great deal to ask the core team, who are all volunteers, to properly maintain and review code in the latest version and we cannot afford to split their efforts across several different versions. Furthermore, we have a proper deprecation policy now to ensure that developers should be able to advance to future versions without critical blockers. Of course, anyone so inclined, is able to maintain their own fork of SproutCore at any release version.