Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add ability to pass multiple keywords #13

Merged
merged 2 commits into from
Nov 19, 2017
Merged

Add ability to pass multiple keywords #13

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ebad0e8
Add ability to pass multiple keywords
mischah Nov 3, 2017
00cae4a
Use more verbose variable names
mischah Nov 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,15 @@ const got = require('got');
const registryUrl = require('registry-url');

function get(keyword, options) {
if (typeof keyword !== 'string') {
return Promise.reject(new TypeError('Keyword must be a string'));
if (typeof keyword !== 'string' && !Array.isArray(keyword)) {
return Promise.reject(new TypeError('Keyword must be either a string or an array of strings'));
}

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', '+');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this to »clever«? 😎

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@mischah mischah Nov 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


const url = `${registryUrl()}-/v1/search?text=keywords:${keyword}&size=${options.size}`;

Expand Down
14 changes: 14 additions & 0 deletions test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {
Copy link
Contributor Author

@mischah mischah Nov 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would love to test the part of the URL separately.
But that would mean to move some code to lib/search-url.js, right?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean you want to test https://github.com/mischah/npm-keyword/blob/ebad0e848901f4d5495e9d605ffbfafd3ae6f528/index.js#L16 separately? For what purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

const cnt1 = await m.count('gulpplugin');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cnt1 is not a readable variable name. Use something more verbose.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

const cnt2 = await m.count(['gulpplugin', 'sass', 'css']);

t.true(cnt2 > 0);
t.true(cnt2 < cnt1);
});

test('npmKeyword.count() using wrong type for keywords parameter', async t => {
const error = await t.throws(m.count({keyword: 'gulpplugin'}));

t.is(error.message, 'Keyword must be either a string or an array of strings');
});