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

feat(check-eslint): Check for ESLint peerDependency being met #290

Closed
wants to merge 1 commit into from

Conversation

ta2edchimp
Copy link
Collaborator

@ta2edchimp ta2edchimp commented Feb 23, 2018

Before anything else, checks that ESLint is installed and present in a sufficient version, exit with code 1 otherwise.

This is currently wip — I just threw it together this evening and wanted to collect some opinions.

There are some minor issues reported by the linter: almost the whole content of bin/diff.js moved into the eslint-check's else-branch and with it all the function declarations. I think that whole thing should be refactored soon anyways, so I'd think it's not that big of a deal ...


I planned to make use of yargs feature to print out usage instructions too (in case of insufficient arguments), but that would be out of scope for this PR.


PS: If not already known, appending ?w=0 to the url for the commits of this PR (like here) ignores whitespace changes and makes this PR much more easy to read.

if (rules.length < 1) {
return;
}
if (checkEslint.check() === false) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

couldn't we just require('eslint') and make it a peer dep?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It currently is a peer dep and we just require it in lib/rule-finder, but in my experience peer deps are not installed automatically so I thought it would be good to have a nice warning.
Eventually I want to avoid confusion as in #285

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

... and instead of just requiring eslint (that is, in a try-catch, to check for it), this draft of an implementation also pulls the corresponding information from the package.json and tests if the installed version satisfies it.

On a second thought, that might be a good idea to even make it a separate, general purpose module and just use that (and concentrate on core functionality of this tool).
What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that letting the require throw naturally, and expecting users to run npm ls themselves, is fine - i don’t see much value in an explicit check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok. I have to admit, I didn‘t realize this check would be as bloated as it got already (and there still is erroneously assumed, the package.json to be tested again would be found at process.cwd, so it would be even more bloated, if done properly).
I‘ll happily close this then.

But to hijack this discussion a little bit: What about a proper usage information (using the functionality provided by yargs)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That part sounds awesome; I love yargs and its auto-help feature.

@ta2edchimp ta2edchimp closed this Feb 24, 2018
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.

2 participants