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

Conversation

graingert
Copy link
Contributor

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

avoid problems with ./ham/ -> ./ham/index.js by simply
consuming '..', path segment pairs.
seems to not normalize trailing path seps.

./foo -> foo
./ -> ./
. -> .
.. -> ..
../ -> ../
@coveralls
Copy link

coveralls commented Aug 8, 2017

Coverage Status

Coverage decreased (-0.02%) to 95.95% when pulling a3cc1af on graingert:no-useless-path-segments into dd28130 on benmosher:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 95.95% when pulling a3cc1af on graingert:no-useless-path-segments into dd28130 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 95.95% when pulling a3cc1af on graingert:no-useless-path-segments into dd28130 on benmosher:master.

@coveralls
Copy link

coveralls commented Aug 8, 2017

Coverage Status

Coverage decreased (-0.03%) to 95.934% when pulling 2657c65 on graingert:no-useless-path-segments into dd28130 on benmosher:master.

@coveralls
Copy link

coveralls commented Aug 8, 2017

Coverage Status

Coverage decreased (-0.03%) to 95.934% when pulling 1c1f323 on graingert:no-useless-path-segments into dd28130 on benmosher:master.

1 similar comment
@coveralls
Copy link

coveralls commented Aug 8, 2017

Coverage Status

Coverage decreased (-0.03%) to 95.934% when pulling 1c1f323 on graingert:no-useless-path-segments into dd28130 on benmosher:master.

@graingert
Copy link
Contributor Author

@benmosher not sure why it's still failing on appveyor

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks! Could you add a lot more test cases, with things like ../foo/../../bar/foo/baz etc?

It'd also be good to warn on /absolute/path/to/foo when it in fact should be './foo and similar.

import moduleVisitor from 'eslint-module-utils/moduleVisitor'

function toRel(rel, sep) {
const rSep = escapeRegExp(sep)
Copy link
Member

Choose a reason for hiding this comment

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

is there a way this could be done without having to escape regexes, or use them at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah it can be removed, this is only used on the posix side now.

@graingert
Copy link
Contributor Author

It'd also be good to warn on /absolute/path/to/foo when it in fact should be './foo and similar.

Maybe on the no absolute rule?

@ljharb
Copy link
Member

ljharb commented Aug 8, 2017

If I want to permit absolute paths, but not ones that could instead be relative, this one seems the appropriate place.

@graingert
Copy link
Contributor Author

Would need to know how far up is too far for absolute paths.

'../../../../users/ljharb/Downloads/bower/jquery-ui'

As that escapes out of the project

@ljharb
Copy link
Member

ljharb commented Aug 9, 2017

Right, you'd locate the package.json, and consider that the root - anything within that should be relative instead of absolute.

When phrased like that, however, it might make sense as an option to no-absolute-path :-D

@coveralls
Copy link

coveralls commented Aug 9, 2017

Coverage Status

Coverage increased (+0.07%) to 96.032% when pulling f7ef6ea on graingert:no-useless-path-segments into dd28130 on benmosher:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 96.032% when pulling f7ef6ea on graingert:no-useless-path-segments into dd28130 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 96.032% when pulling f7ef6ea on graingert:no-useless-path-segments into dd28130 on benmosher:master.

**/
function toRel(rel) {
const stripped = rel.replace(/\/$/g, '')
return stripped.match(/^((\.\.)|(\.))($|\/)/) ?
Copy link
Member

Choose a reason for hiding this comment

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

since we're not using the match object here, let's use regex.test(str) instead.

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.

@graingert
Copy link
Contributor Author

@danny-andrews @ljharb is this ready to go?

@danny-andrews
Copy link
Contributor

Ready on my end!

@ljharb
Copy link
Member

ljharb commented Dec 19, 2017

I'm going to rebase this soon and merge it.

@graingert
Copy link
Contributor Author

Hype

@ljharb ljharb force-pushed the no-useless-path-segments branch 2 times, most recently from 61bce61 to 3090e84 Compare December 19, 2017 03:49
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 96.15% when pulling 3090e84 on graingert:no-useless-path-segments into fa02415 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 96.15% when pulling 3090e84 on graingert:no-useless-path-segments into fa02415 on benmosher:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 96.15% when pulling 3090e84 on graingert:no-useless-path-segments into fa02415 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 96.15% when pulling 3090e84 on graingert:no-useless-path-segments into fa02415 on benmosher:master.

@coveralls
Copy link

coveralls commented Dec 19, 2017

Coverage Status

Coverage increased (+0.06%) to 96.15% when pulling 3090e84 on graingert:no-useless-path-segments into fa02415 on benmosher:master.

@ljharb ljharb merged commit f57f987 into import-js:master Dec 19, 2017
@ljharb
Copy link
Member

ljharb commented Dec 19, 2017

@benmosher do you have the release process documented anywhere? If so, I'll cut a release; if not, would you mind? :-D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants