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

add no-useless-path-segments rule Fixes #471 #912

Merged
merged 13 commits into from
Dec 19, 2017
1 change: 1 addition & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export const rules = {
'no-dynamic-require': require('./rules/no-dynamic-require'),
'unambiguous': require('./rules/unambiguous'),
'no-unassigned-import': require('./rules/no-unassigned-import'),
'no-useless-path-segments': require('./rules/no-useless-path-segments'),

// export
'exports-last': require('./rules/exports-last'),
Expand Down
91 changes: 91 additions & 0 deletions src/rules/no-useless-path-segments.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
/**
* @fileOverview Ensures that there are no useless path segments
* @author Thomas Grainger
*/

import path from 'path'
import sumBy from 'lodash/sumBy'
import resolve from 'eslint-module-utils/resolve'
import moduleVisitor from 'eslint-module-utils/moduleVisitor'

/**
* convert a potentially relative path from node utils into a true
* relative path.
*
* ../ -> ..
* ./ -> .
* .foo/bar -> ./.foo/bar
* ..foo/bar -> ./..foo/bar
* foo/bar -> ./foo/bar
*
* @param rel {string} relative posix path potentially missing leading './'
* @returns {string} relative posix path that always starts with a ./
**/
function toRel(rel) {
const stripped = rel.replace(/\/$/g, '')
return /^((\.\.)|(\.))($|\/)/.test(stripped) ? stripped : `./${stripped}`
}

function normalize(fn) {
return toRel(path.posix.normalize(fn))
}

const countRelParent = x => sumBy(x, v => v === '..')

module.exports = {
meta: { fixable: 'code' },

create: function (context) {
const currentDir = path.dirname(context.getFilename())

function checkSourceValue(source) {
const { value } = source

function report(proposed) {
context.report({
node: source,
message: `Useless path segments for "${value}", should be "${proposed}"`,
fix: fixer => fixer.replaceText(source, JSON.stringify(proposed)),
})
}

if (!value.startsWith('.')) {
return
}

const resolvedPath = resolve(value, context)
const normed = normalize(value)
if (normed !== value && resolvedPath === resolve(normed, context)) {
return report(normed)
}

if (value.startsWith('./')) {
return
}

if (resolvedPath === undefined) {
return
}

const expected = path.relative(currentDir, resolvedPath)
const expectedSplit = expected.split(path.sep)
const valueSplit = value.replace(/^\.\//, '').split('/')
const valueNRelParents = countRelParent(valueSplit)
const expectedNRelParents = countRelParent(expectedSplit)
const diff = valueNRelParents - expectedNRelParents

if (diff <= 0) {
return
}

return report(
toRel(valueSplit
.slice(0, expectedNRelParents)
.concat(valueSplit.slice(valueNRelParents + diff))
.join('/'))
)
}

return moduleVisitor(checkSourceValue, context.options[0])
},
}
1 change: 1 addition & 0 deletions tests/files/bar/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default 4
Empty file added tests/files/index.js
Empty file.
55 changes: 55 additions & 0 deletions tests/src/rules/no-useless-path-segments.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import { test } from '../utils'
import { RuleTester } from 'eslint'

const ruleTester = new RuleTester()
const rule = require('rules/no-useless-path-segments')

function runResolverTests(resolver) {
ruleTester.run(`no-useless-path-segments (${resolver})`, rule, {
valid: [
test({ code: 'import "./malformed.js"' }),
test({ code: 'import "./test-module"' }),
test({ code: 'import "./bar/"' }),
test({ code: 'import "."' }),
test({ code: 'import ".."' }),
test({ code: 'import fs from "fs"' }),
],

invalid: [
test({
code: 'import "./../files/malformed.js"',
errors: [ 'Useless path segments for "./../files/malformed.js", should be "../files/malformed.js"'],
}),
test({
code: 'import "./../files/malformed"',
errors: [ 'Useless path segments for "./../files/malformed", should be "../files/malformed"'],
}),
test({
code: 'import "../files/malformed.js"',
errors: [ 'Useless path segments for "../files/malformed.js", should be "./malformed.js"'],
}),
test({
code: 'import "../files/malformed"',
errors: [ 'Useless path segments for "../files/malformed", should be "./malformed"'],
}),
test({
code: 'import "./test-module/"',
Copy link
Member

Choose a reason for hiding this comment

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

We should test all of these cases:

  1. foo/index.something exists → path should be "foo"
  2. foo.something exists → path should be "foo"
  3. both foo/index.something and foo.something exist → foo/ and foo are different, and the difference is important, and this rule should not warn on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

foo.something exists → path should be "foo"

I think that should be handled by https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/extensions.md

Copy link
Member

Choose a reason for hiding this comment

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

yes, that should handle whether there's an extension or not - i'm saying that "whether this rule can warn on a path or not" actually depends on what's on disk, because the trailing slash is meaningful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a test case for catching multiple slashes, e.g. foo//something? See #915 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

That's a good idea.

errors: [ 'Useless path segments for "./test-module/", should be "./test-module"'],
}),
test({
code: 'import "./"',
errors: [ 'Useless path segments for "./", should be "."'],
}),
test({
code: 'import "../"',
errors: [ 'Useless path segments for "../", should be ".."'],
}),
test({
code: 'import "./deep//a"',
errors: [ 'Useless path segments for "./deep//a", should be "./deep/a"'],
}),
],
})
}

['node', 'webpack'].forEach(runResolverTests)