From 02cda17620aac8504f19cac879c7302301e8d123 Mon Sep 17 00:00:00 2001 From: Esteban Ordano Date: Wed, 10 Dec 2014 13:33:46 -0300 Subject: [PATCH] Refactor Contributing --- CONTRIBUTING.md | 259 ++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 207 insertions(+), 52 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 39a72dba4..b586a43cf 100644 --- a/CONTRIBUTING.md +++ b/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. +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. -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 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)") ``` -Then clone your fork on your machine: -``` -git clone git@github.com:user/bitcore && cd bitcore/ +### 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); ``` -Add the official repo as a remote, to track our changes: -``` -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! - - -