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.

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

Update search tests and lintings #40

Merged
merged 3 commits into from
Oct 21, 2016
Merged

Update search tests and lintings #40

merged 3 commits into from
Oct 21, 2016

Conversation

vutran
Copy link
Member

@vutran vutran commented Oct 20, 2016

This extends on @W0lfw00d's awesome search PR that was just merged.

  • Cleans up some linting issues
  • Create tests for new search endpoint
  • Include package description in search results
  • Cleans up the resolved search results structure

@coveralls
Copy link

Coverage Status

Coverage increased (+3.6%) to 100.0% when pulling 0d1d004 on fix-linting-tests into 8141084 on develop.

@vutran vutran merged commit 524769c into develop Oct 21, 2016
@vutran vutran deleted the fix-linting-tests branch October 21, 2016 05:49
@@ -6,6 +6,7 @@ import {
ERR_MODULE_NOT_FOUND,
ERR_MODULE_DISABLED,
ERR_MODULE_REMOVE_FAILED,
ERR_MODULE_SEARCH_FAILED,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one still need to be added to 'errors/index.js'

Copy link
Member Author

Choose a reason for hiding this comment

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

Woops, looks like I left that out as well! Will get to this piece tonight otherwise feel free to send a quick PR. 👍

const results = packages.map(pkg => pkg.name[0]);
resolve(results);
} else {
if (!Array.isArray(packages) || !packages.length) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If no packages are found for the search it will result in a reject (and an error in the cli) (because of !packages.length) and maybe you just want to reject if the search actually fails.

* @param {String} q - The keyword to search
* @return {String}
*/
const getSearchUrl = q => `https://npmsearch.com/query?q=${q}%20AND%20(keywords:dext-theme%20OR%20keywords:dext-plugin)&fields=name,description`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't work on my machine for some reason...

Error: Protocol "https:" not supported. Expected "http:"
    at new ClientRequest (_http_client.js:55:11)
    at Object.exports.request (http.js:31:10)
    at Object.exports.get (http.js:35:21)
    at Promise (/Users/wolfwood/dev/dext-cli/node_modules/dext-core-utils/src/utils/search.js:23:15)
    at searchPackages (/Users/wolfwood/dev/dext-cli/node_modules/dext-core-utils/src/utils/search.js:19:29)
    at Promise (/Users/wolfwood/dev/dext-cli/node_modules/dext-core-utils/src/api/index.js:29:3)
    at Object.search (/Users/wolfwood/dev/dext-cli/node_modules/dext-core-utils/src/api/index.js:28:30)
    at Args.args.command (/Users/wolfwood/dev/dext-cli/cli.js:12:14)
    at Args.runCommand (/Users/wolfwood/dev/dext-cli/node_modules/args/dist/index.js:327:37)
    at Args.parse (/Users/wolfwood/dev/dext-cli/node_modules/args/dist/index.js:429:12)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah! This is my fault. I've updated the endpoint to the secure protocol. We'll just need to require the https module instead of http. Feel free to send a new PR for this. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants