ericblade / mws-simple

nodejs Amazon MWS (Merchant Web Services) API in ~150 lines of code
MIT License
8 stars 5 forks source link

Promisified the `request` method #28

Closed avrtau closed 5 years ago

avrtau commented 5 years ago

FEATURE: request method now returns a Promise if no callback is specified.

BREAKING CHANGE: Callback for makeRequest parameter change from (error, result, headers) to (error, {result, headers}).

Documentation is updated

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 133


Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/makeRequest.js 0 3 0.0%
mws-simple.js 1 6 16.67%
<!-- Total: 1 9 11.11% -->
Files with Coverage Reduction New Missed Lines %
lib/syncWriteToFile.js 2 33.33%
lib/makeSignature.js 2 60.0%
lib/getContentType.js 3 12.5%
lib/ServerError.js 4 14.29%
mws-simple.js 9 34.15%
lib/makeRequest.js 10 11.11%
<!-- Total: 30 -->
Totals Coverage Status
Change from base Build 115: -55.7%
Covered Lines: 21
Relevant Lines: 66

💛 - Coveralls
ericblade commented 5 years ago

oh, great, thank you! That looks good, I'll check it out in the morning. Also, that's a good idea on the parameters change, wish I'd've thought of it when I bumped the major version number last.. ooh well. :)

ericblade commented 5 years ago

Hey @avrtau , I updated the tests to match the new API, added a test for Promise, bumped version, and put you into the Contributors list. I'd appreciate it if you'd cherry-pick this change, or write something similar yourself, and attach it to this pull request. thanks!

https://github.com/ericblade/mws-simple/commit/3e6163804fcd55bdbb80cd74299314cf1e901152

ericblade commented 5 years ago

oh, also, the reason why coverage is complaining is because travis-ci for some reason does not provide API credentials unless I do the pull request. So don't worry about that, I always manually run tests before i accept anything :)

avrtau commented 5 years ago

Hey @avrtau , I updated the tests to match the new API, added a test for Promise, bumped version, and put you into the Contributors list. I'd appreciate it if you'd cherry-pick this change, or write something similar yourself, and attach it to this pull request. thanks!

3e61638

I am not 100% sure what you are asking me to do - not very familiar with github yet. Should I fork the newest version and add a pull request there?

ericblade commented 5 years ago

@avrtau probably quickest to do would be

git remote add ericblade git@github.com:ericblade/mws-simple.git
git fetch ericblade
git cherry-pick 3e61638
git push origin master

that should take my commit 3e61638 and put it into your pull req. That way the fixes to the test go into the pull request together with the changes, instead of having them separate. :)

avrtau commented 5 years ago

@avrtau probably quickest to do would be

git remote add ericblade git@github.com:ericblade/mws-simple.git
git fetch ericblade
git cherry-pick 3e61638
git push origin master

that should take my commit 3e61638 and put it into your pull req. That way the fixes to the test go into the pull request together with the changes, instead of having them separate. :)

when I fetch:

git@github.com: Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

EDIT: never mind. Changed git@github.com:ericblade/mws-simple.git to https://github.com/ericblade/mws-simple.git

avrtau commented 5 years ago

codebeat is relentless

ericblade commented 5 years ago

Don't worry about codebeat :-) or the coverage warnings. Codebeat isn't very configurable, and most of what it warns about aren't really worth worrying about (very low on the error threshold)

Thanks for updating the pull!