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

Merged
merged 1 commit into from Aug 10, 2016

Projects

None yet

6 participants

@mysticatea
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

LGTM

@jquerybot jquerybot added the CLA: Valid label Aug 8, 2016
@gyandeeps
Member

LGTM, waiting for others to review.

@platinumazure
Member

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

@ilyavolodin ilyavolodin and 1 other commented on an outdated diff Aug 9, 2016
@@ -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)) {
@ilyavolodin
ilyavolodin Aug 9, 2016 Member

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.

@mysticatea
mysticatea Aug 9, 2016 Member

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

@ilyavolodin
Member

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

@mysticatea
Member
mysticatea commented Aug 9, 2016 edited

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

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

LGTM

@mysticatea
Member

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

@ilyavolodin
Member

LGTM, but waiting another day for others to look

@platinumazure
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

4 of 5 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
jQuery Foundation CLA All authors have signed the CLA
Details
@mysticatea mysticatea deleted the chore/eslint-plugin-node branch Aug 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment