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

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

Merged
merged 13 commits into from
Dec 19, 2017
Merged

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

merged 13 commits into from
Dec 19, 2017

Conversation

graingert
Copy link
Contributor

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

@graingert @ljharb
add no-useless-path-segments rule Fixes import-js#471
9806019
@graingert @ljharb
add failing test
0b9e333
@graingert @ljharb
normalize path first, then count '../'s
ec8bfca 
avoid problems with ./ham/ -> ./ham/index.js by simply
consuming '..', path segment pairs.
@graingert @ljharb
path.normalize sometimes . eg when there's only one .
57eb999 
seems to not normalize trailing path seps.

./foo -> foo
./ -> ./
. -> .
.. -> ..
../ -> ../
@graingert @ljharb
add no-useless-path-segments fixer
80eba07
@coveralls
Copy link

coveralls commented Aug 8, 2017 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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

ljharb
ljharb requested changes Aug 8, 2017
View reviewed changes
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.

src/rules/no-useless-path-segments.js Outdated
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 •
edited
Loading

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.

ljharb
ljharb requested changes Aug 10, 2017
View reviewed changes
src/rules/no-useless-path-segments.js Outdated
**/
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.

tests/src/rules/no-useless-path-segments.js
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.

This was referenced Aug 16, 2017
Closed
Closed
Closed
@graingert
Copy link
Contributor Author

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

@danny-andrews
Copy link
Contributor

Ready on my end!

@ljharb ljharb mentioned this pull request Dec 19, 2017
Closed
@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 •
edited
Loading

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

@graingert graingert deleted the no-useless-path-segments branch December 19, 2017 11:02
@ljharb ljharb mentioned this pull request Jan 4, 2018
Closed
@graingert graingert mentioned this pull request Jan 5, 2018
Closed
@renovate renovate bot mentioned this pull request Feb 22, 2018
Merged
@ljharb ljharb mentioned this pull request Feb 22, 2018
Closed
@renovate renovate bot mentioned this pull request Mar 22, 2018
Merged
This was referenced Aug 17, 2021
Open
Open
Open
@snyk-bot snyk-bot mentioned this pull request Sep 4, 2021
Open
@MaxMood96 MaxMood96 mentioned this pull request May 13, 2022
Open
@snyk-bot snyk-bot mentioned this pull request May 14, 2022
Open
@MaxMood96 MaxMood96 mentioned this pull request May 14, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danny-andrews danny-andrews danny-andrews left review comments

@ljharb ljharb ljharb approved these changes

@benmosher benmosher Awaiting requested review from benmosher

@jfmengels jfmengels Awaiting requested review from jfmengels

Assignees
No one assigned
Labels
rule proposal semver-minor
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@graingert @coveralls @ljharb @danny-andrews