pat310 / google-trends-api

An API layer on top of google trends
https://www.npmjs.com/package/google-trends-api
MIT License
896 stars 178 forks source link

Support for Multiple Keywords #54

Closed Dayjo closed 7 years ago

Dayjo commented 7 years ago

This pull request implements the ability to use an array of keywords as well as just a single string.

This doesn't update any documentation, I want to make sure that the code is acceptable first!

Any suggestions / improvements I can make please let me know @pat310

Fixes #53

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-4.09%) to 89.655% when pulling 3b78ebe3ed0f6381344de0b350f27a0d825b7157 on pallant:multiple-keywords into 12aa6e6db6819182ee38a7e68974b7fd2368b7ae on pat310:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-4.09%) to 89.655% when pulling 3999cc894bd1735c8950403b65e9fc284e1b0141 on pallant:multiple-keywords into 12aa6e6db6819182ee38a7e68974b7fd2368b7ae on pat310:master.

Dayjo commented 7 years ago

Any assistance on the code coverage would be appreciated, not 100% sure what I need to do to fix it.

pat310 commented 7 years ago

@Dayjo Cool! I think we should move this whole thing to a new function in utilites.js and that will return the comparisonItem array. getResults will call that function at line 116 or assign it to a variable comparisonItem to be used at line 116. Then add some tests for that new function in utilities.spec.js, that should get the code coverage back up. Let me know if you need help

Dayjo commented 7 years ago

Thanks for this is, great feedback thank you 👍 I shall endeavour to do all of the above.

Dayjo commented 7 years ago

@pat310 inline comments have disappeared cause I pushed my latest changes (failing).

As you can see, the syntax checker is failing on the {...obj part unfortunately so I can't build/test.

Dayjo commented 7 years ago

@pat310 I added "transform-es2015-destructuring", "transform-object-rest-spread" to the babel plugins to fix the syntax checking for object spread.

Also added two tests for the formatKeywords function one to check for normal single keyword and one for checking an array of keywords. Do you have any suggestions for what else I can test on this?

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.4%) to 92.308% when pulling 482c61ef5e7860bce90edebe4037f39cf8feb564 on pallant:multiple-keywords into 12aa6e6db6819182ee38a7e68974b7fd2368b7ae on pat310:master.

Dayjo commented 7 years ago

Eh, not sure why the coverage has dropped, pretty sure my new function is covered.

pat310 commented 7 years ago

Looks good @Dayjo. Just a few last comments and I think it's good to go

pat310 commented 7 years ago

Do you want to update the README as well?

turnerniles commented 7 years ago

@Dayjo Are you nightcrawler in your profile picture?

Dayjo commented 7 years ago

Tests seem to pass but I do get an error here;

image

Coverage is still down too :/

@turnerniles Haha, no, I just blue myself.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.6%) to 92.143% when pulling a6069df54134bc24152b5d65c397042bbe34eff0 on pallant:multiple-keywords into 12aa6e6db6819182ee38a7e68974b7fd2368b7ae on pat310:master.