search

pwmckenna / node-travis-encrypt

node module to encrypt environment variables for travis-ci
npmjs.org/package/travis-encrypt
46 stars 12 forks source link

Change module API + QA improvements #19

Closed rexxars closed 8 years ago

rexxars commented 8 years ago

This PR changes the module API to accept an object instead of individual parameters, as discussed in #10.

I also took the liberty of adding tests for the CLI tool, refactored some code to minimize code duplication and replaced the aging JSHint configuration with ESLint. In the CLI tests, I've used spawnSync, which is only available in Node >= 0.12. I think it's fine to encourage people to upgrade, so I've set the minimum version in package.json to match, but the tests could be refactored if we think 0.10 support is still important.

Obviously this should be released as 2.0 since this introduces breaking changes. To make it easier to upgrade, I've added a changelog.

Sorry about the big PR, I probably should have split this one into smaller chunks.

rexxars commented 8 years ago

On request from @rstacruz I've changed from eslint to semistandard, which involved changing from 4 spaces to 2.

rstacruz commented 8 years ago

makes a lotta sense. :+1: for me.

pwmckenna commented 8 years ago

this is awesome work. api is much improved. :100: the node version doesn't seem like that big of a deal. will it actually block install of this module if you're using an old version of node or does it just warn? either way I'd say :shipit:

rexxars commented 8 years ago

NPM says this field is advisory only, so I guess it'll just warn. Since only the tests are using 0.12 features, I decided it was kind of stupid to require it for the whole package. I refactored the tests to use async spawn and added support for 0.10 again. Merging.