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

Chore: use eslint-plugin-node (refs #6407) #6862

Merged
merged 1 commit into from Aug 10, 2016
Merged

Chore: use eslint-plugin-node (refs #6407) #6862

merged 1 commit into from Aug 10, 2016

Conversation

mysticatea
Copy link
Member

Refs #6407.

This PR enables eslint-plugin-node to lint our codebase.

plugins:
    - node
extends:
    - "plugin:node/recommended"

This would help us:

  • To early warn a use of unsupported ecma features in Node 4.
  • To warn a use of deprecated API.
  • To early warn invalid paths in require(...).
  • To early warn paths which are not included in dependencies in require(...).

Also, this PR removes some uses of deprecated API.

  • util.isArray
  • fs.existsSync

@eslintbot
Copy link

LGTM

@gyandeeps
Copy link
Member

LGTM, waiting for others to review.

@platinumazure
Copy link
Member

Do the new path-util functions need tests? (Maybe I missed them?)

ilyavolodin
ilyavolodin reviewed Aug 9, 2016
View reviewed changes
lib/cli.js
@@ -82,7 +83,7 @@ function printResults(engine, results, format, outputFile) {
if (outputFile) {
const filePath = path.resolve(process.cwd(), outputFile);

if (fs.existsSync(filePath) && fs.statSync(filePath).isDirectory()) {
if (pathUtil.isDirectory(filePath)) {
Copy link
Member

Choose a reason for hiding this comment

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

In a bunch of places when dealing with config we use shelljs to check if it's a file, I think there was a reason why we did that instead of using fs. @gyandeeps do you remember? Should we use shelljs everywhere or fs? It would be good if we just use pathUtil for this everywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch.
If I replace pathUtil by shell.test("-d" and shell.test("-f", I don't need to add new functions.

@ilyavolodin
Copy link
Member

Agree with @platinumazure Needs tests for isFile and isDirectory, otherwise LGTM.

@mysticatea
Copy link
Member Author

mysticatea commented Aug 9, 2016 •
edited

Ah, sorry. I have forgotten tests for the added functions.

@mysticatea
Chore: use eslint-plugin-node (refs #6407)
104f831 
- To warn unsupported ecma features of Node 4.
- To prevent a use of deprecated API.
- etc.
@eslintbot
Copy link

LGTM

@mysticatea
Copy link
Member Author

I removed pathUtil.isFile and pathUtil.isDirectory, then I use shell.test.

@ilyavolodin
Copy link
Member

LGTM, but waiting another day for others to look

@platinumazure
Copy link
Member

LGTM. Pretty sure the AppVeyor failure is the too many arguments issue that gyandeeps has since fixed, so hopefully this is safe to merge.

@gyandeeps gyandeeps merged commit e8cb7f9 into master Aug 10, 2016
@mysticatea mysticatea deleted the chore/eslint-plugin-node branch August 11, 2016 12:18
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mysticatea @eslintbot @gyandeeps @platinumazure @ilyavolodin @jquerybot