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

New: Ability to search npm packages #785

Closed
wants to merge 5 commits into from

Conversation

sarvaje
Copy link
Contributor

@sarvaje sarvaje commented Jan 27, 2018

Pull request checklist

Make sure you:

For non-trivial changes, please make sure you also:

  • Added/Updated related documentation.
  • Added/Updated related tests.

Short description of the change(s)

This PR allow us to get the npm packages with rules/parsers/etc. directly from npm, and it will be useful for futures updates in sonarwhal.

export const getCoreRulesFromNpm = async () => {
const rules = await getNpmPackages('@sonarwhal/rule');

return filterPackages(rules, '@sonarwhal/rule');
Copy link
Member

Choose a reason for hiding this comment

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

Can you put a comment explaining why we are filtering even though you are searching for @sonarwhal/rule?

Maybe we could also do a search just by @sonarwhal and then do the grouping in memory by rule/parser/connector/etc.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if it is because I'm doing something weird or not, but the search doesn't work like you expect to work. With the packages there is right now, search for @sonarwhal or sonarwhal return the same elements (and not, not all the packages start with @sonarwhal`.

About the filter, I will write some explanation, but look for @sonarwhal/rule doesn't have to return only packages that start with @sonarwhal/rule :(

src/lib/types.ts Outdated
@@ -74,3 +74,17 @@ export interface IORA {
fail(): void;
text: string;
}

export type NpmMaintainer = {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add some descriptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@molant
Copy link
Member

molant commented Jan 30, 2018

This is in preparation for #748. We will merge this PR once the other one is merged.

The idea is to replace the results of getting the installed core rules with the npm search. The reason is that there will not be any rule installed by default.

Some things to take into account while this PR continues:

  • We will probably have issues when installing sonarwhal globally because we might need sudo powers to install the rules. I think that ideally we should have a post-install task so the user can at least install a few ones during the initial process.
  • If the user runs --init when installed globally (after install) then we can tell them to run sudo npm rulesNames if the process fails
  • This will probably affect the work of Allow CLI to be run without .sonarwhalrc in current directory #718 so @poshaughnessy you might want to hold on that until we finish this refactor (or we can take care of migrating if it lands before the rest)

@sarvaje, any other thing that we discussed that I forgot?

@poshaughnessy
Copy link
Contributor

@molant re. #718, no worries - sorry, I got delayed by other work again anyway! So holding off until this refactor is finished sounds good to me. I can start investigating / preparing in the meantime.

@alrra alrra force-pushed the master branch 2 times, most recently from f5613b2 to 09aad7c Compare February 1, 2018 03:20
@sarvaje sarvaje force-pushed the search-npm branch 2 times, most recently from d91817b to 407bdff Compare February 6, 2018 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants