Refactor Contributing

CONTRIBUTING.md

@@ -1,84 +1,239 @@
 Contributing to Bitcore
 =======
 
-Are you a developer looking to learn more about bitcoin? 
-Bitcore is a great opportunity to do so, and give back to
-the community. At BitPay we encourage any developer to read the source 
-code and help us improve it by fixing bugs, implementing
-new exciting features, and testing existing code. 
-
-Pull requests are the standard mechanism by which you contribute code to open-source projects.
-To do so, start by forking our repo on GitHub. Go to 
-[github.com/bitpay/bitcore](https://github.com/bitpay/bitcore)
-and click the 'Fork' button.  You'll get your own fork of the repository which will look something like this:
-```
-https://github.com/user/bitcore
-```
+We're working hard to make *bitcore* the easier and most powerful javascript
+library for working with bitcoin. Our goal is to have *bitcore* be a library
+that can be used by anyone interested on bitcoin, and level the expertise
+differences with a great design and documentation.
 
-Then clone your fork on your machine:
-```
-git clone git@github.com:user/bitcore && cd bitcore/
+We have a pretty strict set of guidelines for contributing.
+
+## Quick checklist
+
+Make sure:
+
+* `gulp lint` doesn't complain about your changes
+* `gulp test` passes all the tests
+* `gulp coverage` covers 100% of the branches of your code
+
+## Design Guidelines
+
+These are some global design goals in bitcore that any change must adhere to.
+
+### D1 - Naming Matters
+
+We take our time with picking names. Code is going to be written once, and read
+hundreds of times.
+
+We were inspired to name this rule first due to Uncle Bob's great work *Clean
+Code*, which has a whole chapter on this subject.
+
+In particular, you may notice that some names in this library are quite long
+for the average javascript user. That's because we better have a long but
+comprehensible name rather than an abbreviation that might confuse new users.
+
+### D2 - Tests
+
+Write a test for all your code. We encourage Test Driven Development so we know
+when our code is right. We migrated from our original code base with a 80% test
+coverage and are targeting 100% as we move towards our 1.0 release.
+
+### D3 - Robustness Principle
+
+*Be conservative in what you send, be liberal in what you accept.*
+
+Interfaces accept as much types of arguments as possible, so there's no mental
+tax on using them: we want to avoid questions such as "should I use a string
+here or a buffer?", "what happens if I'm not sure if the type of this variable
+is an Address instance or a string with it encoded in base-58?" or "what kind
+of object will I receive after calling this function?".
+
+Accept a wide variety of usages and arguments, always return an internal kind
+of object. For example, the class `PublicKey` can accept strings or buffers
+with a DER encoded public key (either compressed or uncompressed), another
+PublicKey, a PrivateKey, or a Point, an instance of the `elliptic.js` library
+with the point in bitcoin's elliptic curve that represents the public key.
+
+### D4 - Consistency Everywhere
+
+Consistency on the way classes are used is paramount to allow an easier
+understanding of the library.
+
+## Style Guidelines
+
+The design guidelines have quite a high abstraction level. These style
+guidelines are easier to detect and apply, and also more opinionated (the
+design guidelines mentioned above are the way we think about general software
+development and we believe they should be present on any software project).
+
+### G1 - General: No Magic Numbers
+
+Avoid constants in the code as much as possible. Magic strings are also magic
+numbers.
+
+### G2 - General: Internal Objects should be Instances
+
+If a class has a `publicKey` member, for instance, that should be a `PublicKey`
+instance.
+
+### G3 - General: Internal amounts must be integers representing Satoshis
+
+Avoid representation errors by always dealing with satoshis. For conversion for
+frontends, use the `Unit` class.
+
+### G4 - General: Internal network references must be Network instances
+
+A special case for [G2](#g2---general-internal-objects-should-be-instances) all
+network references must be `Network` instances (see `lib/network.js`), but when
+returned to the user, its `.name` property should be used.
+
+### G5 - General: Objects should display nicely in the console
+
+Write a `.inspect()` method so an instance can be easily debugged in the
+console.
+
+### E1 - Errors: Use bitcore.Errors
+
+We've designed a structure for Errors to follow and are slowly migrating to it.
+
+Usage:
+* Errors are generated in the file `lib/errors/index.js` by invoking `gulp
+  errors`.
+* The specification for errors is written in the `lib/errors/spec.js` file.
+* Whenever a new class is created, add a generic error for that class in
+  `lib/errors/spec.js`.
+* Specific errors for that class should subclass that error. Take a look at the
+  structure in `lib/errors/spec.js`, it should be clear how subclasses are
+  generated from that file.
+
+### E2 - Errors: Provide a getValidationError static method for classes
+
+
+### I1 - Interface: Make Code that Fails Early
+
+In order to deal with javascript's weak typing and confusing errors, we ask our
+code to fail as soon as possible when an unexpected input was provided.
+
+There's a module called `util/preconditions`, loosely based on
+`preconditions.js`, based on `guava`, that we use for state and argument
+checking. It should be trivial to use. We recommend using it on all methods, in
+order to improve robustness and consistency.
+
+```javascript
+$.checkState(something === anotherthing, 'Expected something to be anotherthing');
+$.checkArgument(something < 100, 'something', 'must be less than 100');
+$.checkArgumentType(something, PrivateKey, 'something'); // The third argument is a helper to mention the name of the argument
+$.checkArgumentType(something, PrivateKey); // but it's optional (will show up as "(unknown argument)")
 ```
 
-Add the official repo as a remote, to track our changes:
+### I2 - Interface: Permissive Constructors
+
+Most classes have static methods named `fromBuffer`, `fromString`, `fromJSON`.
+Whenever one of those methods is provided, the constructor for that class
+should also be able to detect the type of the arguments and call the
+appropriate method.
+
+### I3 - Interface: Method Chaining
+
+For classes that have a mutable state, most of the methods that can be chained
+*SHOULD* be chained, allowing for interfaces that read well, like:
+
+```javascript
+var transaction = new Transaction()
+    .from(utxo)
+    .to(address, amount)
+    .change(address)
+    .sign(privkey);
 ```
-git remote add bitpay git@github.com:bitpay/bitcore.git
+
+### I4 - Interface: Copy Constructors
+
+Constructors, when provided an instance of the same class, should:
+* Return the same object, if the instances of this class are immutable
+* Return a deep copy of the object, if the instances are mutable
+
+Examples:
+```javascript
+function MyMutableClass(arg) {
+  if (arg instanceof MyMutableClass) {
+    return MyMutableClass._deepCopy(arg);
+  }
+  // ...
+}
+function ImmutableClass(arg) {
+  if (arg instanceof ImmutableClass) {
+    return arg;
+  }
+  // ...
+}
 ```
 
-Create a new branch for the changes you are going to contribute, with a relevant name. Some examples:
+### I5 - Interface: No new keyword for Constructors
+
+Constructors should not require to be called with `new`. This rule is not
+heavily enforced, but is a "nice to have".
+
+```javascript
+function NoNewRequired(args) {
+  if (!(this instanceof NoNewRequired)) {
+    return new NoNewRequired(args);
+  }
+  // ...
+}
 ```
+
+### T1 - Testing: Tests Must be Written Elegantly
+
+Style guidelines are not relaxed for tests. Tests are a good way to show how to
+use the library, and maintaining them is extremely necessary.
+
+Don't write long tests, write helper functions to make them be as short and
+concise as possible (they should take just a few lines each), and use good
+variable names.
+
+### T2 - Testing: Tests Must not be Random
+
+Inputs for tests should not be generated randomly. Also, the type and structure
+of outputs should be checked.
+
+
+## Pull Request Workflow
+
+Our workflow is based on GitHub's pull requests. We use feature branches,
+prepended with: `test`, `feature`, `fix`, `refactor`, or `remove` according to
+the change the branch introduces. Some examples for such branches are:
+```sh
 git checkout -b test/some-module
 git checkout -b feature/some-new-stuff
 git checkout -b fix/some-bug 
 git checkout -b remove/some-file
 ```
 
-Work on your changes: 
-```
-vim somefile.txt
-git add somefile.txt
-git commit -a -m"adding somefile.txt"
-```
-
-When you think your code is ready, update your branch by 
-getting the changes from the main repo first, as there may have been
-changes while you were working:
-```
+We expect pull requests to be rebased to the master branch before merging:
+```sh
+git remote add bitpay git@github.com:bitpay/bitcore.git
 git pull --rebase bitpay master
 ```
-(You may need to solve any conflicts from the rebase at this point.)
 
-Note that we require rebasing your branch instead of mergeing it, for commit readability reasons. 
-
-A final and important step is to run the tests and check they all pass.
-This is done by running `mocha` in the project's directory. You'll also 
-need to check that tests pass in the browser, by running:
-`grunt shell` and opening the `bitcore/test/index.html` file in your
-browser.
+Note that we require rebasing your branch instead of mergeing it, for commit
+readability reasons. 
 
 After that, you can push the changes to your fork, by doing:
-```
+```sh
 git push origin your_branch_name
 git push origin feature/some-new-stuff
 git push origin fix/some-bug
 ```
+Finally go to [github.com/bitpay/bitcore](https://github.com/bitpay/bitcore) in
+your web browser and issue a new pull request.
 
-Finally go to [github.com/bitpay/bitcore](https://github.com/bitpay/bitcore) in your
-web browser and issue a new pull request. GitHub normally recognizes you have pending
-changes in a new branch and will suggest creating the pull request. If it doesn't, you can
-always go to [github.com/bitpay/bitcore/compare](https://github.com/bitpay/bitcore/compare) and
-choose the correct forks and branches. 
-
-Main contributors will review your code and possibly ask for 
-changes before your code is pulled in to the main repository. 
-We'll check that all tests pass, review the coding style, and
-check for general code correctness. If everything is OK, we'll 
-merge your pull request and your code will be part of bitcore.
+Main contributors will review your code and possibly ask for changes before
+your code is pulled in to the main repository.  We'll check that all tests
+pass, review the coding style, and check for general code correctness. If
+everything is OK, we'll merge your pull request and your code will be part of
+bitcore.
 
 If you have any questions feel free to post them to
 [github.com/bitpay/bitcore/issues](https://github.com/bitpay/bitcore/issues).
 
 Thanks for your time and code!
-
-
-