Add ability to pass multiple keywords #13
Conversation
| } | ||
|
|
||
| if (options.size < 1 || options.size > 250) { | ||
| return Promise.reject(new TypeError('Size option must be between 1 and 250')); | ||
| } | ||
|
|
||
| keyword = encodeURIComponent(keyword); | ||
| keyword = encodeURIComponent(keyword).replace('%2C', '+'); |
There was a problem hiding this comment.
Is this to »clever«? 😎
There was a problem hiding this comment.
Why are you doing it? From the docs:
separate multiple keywords with commas
So that should be + => ,, but have you tested whether it supports encoded comma or not?
There was a problem hiding this comment.
The docs are wrong misleading (Going to create a PR to fix thisSee https://github.com/npm/registry/pull/235).
Combining keywords with , acts like a || while combining them with + acts like a &&.
So we need to use a + in our use case.
Compare the following:
keywords:yeoman-generator ➡️ "total": 6994
keywords:yeoman-generator,baumeister ➡️ "total": 6994
keywords:yeoman-generator+baumeister ➡️ "total": 1
keywords:yeoman-generator,-baumeister ➡️ "total": 6993
test.js
Outdated
| @@ -61,3 +61,17 @@ test('npmKeyword.count()', async t => { | |||
| t.is(typeof cnt1, 'number'); | |||
| t.is(cnt2, 0); | |||
| }); | |||
|
|
|||
| test('npmKeyword.count() using an array of keywords', async t => { | |||
There was a problem hiding this comment.
Would love to test the part of the URL separately.
But that would mean to move some code to lib/search-url.js, right?
There was a problem hiding this comment.
You mean you want to test https://github.com/mischah/npm-keyword/blob/ebad0e848901f4d5495e9d605ffbfafd3ae6f528/index.js#L16 separately? For what purpose?
There was a problem hiding this comment.
To make the testing the correct URL creation independent of the API and the response.
I’m not sure if this is necessary 🤔
What do you mean?
| } | ||
|
|
||
| if (options.size < 1 || options.size > 250) { | ||
| return Promise.reject(new TypeError('Size option must be between 1 and 250')); | ||
| } | ||
|
|
||
| keyword = encodeURIComponent(keyword); | ||
| keyword = encodeURIComponent(keyword).replace('%2C', '+'); |
There was a problem hiding this comment.
Why are you doing it? From the docs:
separate multiple keywords with commas
So that should be + => ,, but have you tested whether it supports encoded comma or not?
test.js
Outdated
| @@ -61,3 +61,17 @@ test('npmKeyword.count()', async t => { | |||
| t.is(typeof cnt1, 'number'); | |||
| t.is(cnt2, 0); | |||
| }); | |||
|
|
|||
| test('npmKeyword.count() using an array of keywords', async t => { | |||
There was a problem hiding this comment.
You mean you want to test https://github.com/mischah/npm-keyword/blob/ebad0e848901f4d5495e9d605ffbfafd3ae6f528/index.js#L16 separately? For what purpose?
test.js
Outdated
| @@ -61,3 +61,17 @@ test('npmKeyword.count()', async t => { | |||
| t.is(typeof cnt1, 'number'); | |||
| t.is(cnt2, 0); | |||
| }); | |||
|
|
|||
| test('npmKeyword.count() using an array of keywords', async t => { | |||
| const cnt1 = await m.count('gulpplugin'); | |||
There was a problem hiding this comment.
cnt1 is not a readable variable name. Use something more verbose.
|
This could get another round of review 😗 |
Pretty please 😘 And please let me know if I annoy you with this one. I can imagine that you have tons of other issues, PRs and own ideas you would like to work on. It’s just that I really want this to happen to be able to work on yeoman/yo#544. |
|
Thanks for your patience. I was away on vacation. |
|
No problem. Thanks for taking care. |
This will close #12