ncb000gt / node-es

NodeJS module for ElasticSearch.
103 stars 81 forks source link

Promises & some cleanup #59

Closed dougmoscrop closed 8 years ago

dougmoscrop commented 8 years ago

I hope I wasn't overzealous in these changes; some of it was to get it working on Windows (test/coverage/etc).

Here's a list of things I did in the cleanup commit:

The second commit is to address https://github.com/ncb000gt/node-es/issues/58 in a backwards compatible way.

Thoughts?

Edit: I noticed I did not run the functional tests in the new test script. I'll push a fix now.

dougmoscrop commented 8 years ago

Looks like the coveralls part works, but there was a bit of a decline. I don't know if that's just difference in instrumentation or not - I can poke around and try to get coverage back up.

brozeph commented 8 years ago

In general, I like the direction of this - I was not familiar with thenify, but after looking closely at that library, I like how the module supports backwards compatibility with callback usage as well and it appears to be seamless.

dougmoscrop commented 8 years ago

So the code coverage reduction seems more a byproduct of the change to istanbul than anything else. Based on the coverage report I don't see a way to add much more coverage without getting in to more elaborate / brittle testing scenarios.

brozeph commented 8 years ago

Merging this PR to prepare an NPM release... another PR inbound shortly.