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

Improve ignore paths globifying #539

Closed
wants to merge 1 commit into from

Conversation

laggingreflex
Copy link

Ensures that if only a directory's basename is provided (without the preceeding path, just dirname), it prefixes **/

Currently when only a directory name is provided, it adds a postfix /** which is great for matching the entire tree _if_ the tree is a child of the dir. So path (converted to path/**) can filter paths like path/to/file.js, but if path is actually only a directory's basename it fails to match /some/path/to/file.js. This can be solved by prefixing **/path

Really helpful when only "node_modules" is provided as ignore path (like in .gitignore file).

This could probably have solved #447, which if I followed correctly the solution ultimately was to add paths in similar patterns

    "**/.git/objects/**"
    "**/node_modules/**"

Also adjust the postfix to add just ** instead of /** based on if last char is already a /

@coveralls
Copy link

coveralls commented Oct 8, 2016

Coverage Status

Coverage decreased (-7.8%) to 90.488% when pulling 2594dfd on laggingreflex:subdir-glob into 5d56de2 on paulmillr:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-7.8%) to 90.488% when pulling 2594dfd on laggingreflex:subdir-glob into 5d56de2 on paulmillr:master.

Ensures that if only a directory's basename is provided (without the preceeding path, just `dirname`), it prefixes `**/`

Currently when only a directory name is provided, it adds a postfix `/**` which is great for matching the entire tree ***if*** the tree is a *child* of the dir.  So `path` (converted to `path/**`) can filter paths like `path/to/file.js`, **but** if `path` is actually only a directory's basename it fails to match `/some/path/to/file.js`. This can be solved by prefixing `**/path`

Really helpful when only "node_modules" is provided as ignore path (like in `.gitignore` file) and it's actually some level deeper than current dir.

Also adjust the postfix to add just `**` instead of `/**` based on if last char is already a `/`
@coveralls
Copy link

coveralls commented Oct 8, 2016

Coverage Status

Coverage increased (+0.03%) to 98.314% when pulling e4aff97 on laggingreflex:subdir-glob into 5d56de2 on paulmillr:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 98.314% when pulling e4aff97 on laggingreflex:subdir-glob into 5d56de2 on paulmillr:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 98.314% when pulling e4aff97 on laggingreflex:subdir-glob into 5d56de2 on paulmillr:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 98.314% when pulling e4aff97 on laggingreflex:subdir-glob into 5d56de2 on paulmillr:master.

@es128
Copy link
Contributor

es128 commented Oct 10, 2016

At a glance, this doesn't make sense to me. The relative-ness of the glob paths should match with the way the paths are specified for being watched, and that's how it's output. If you watch with absolute paths, your glob patterns need to reflect the entire absolute path and vice versa.

If something about that isn't working as expected, we can look at it and see if we can solve for the underlying cause to make sure the paths being matched reflect the same basedir as the patterns, but automatically prefixing with ** seems like an invitation for new bugs.

Maybe instead of prefixing with ** we could consider prefixing any relative ignore paths with the glob-parent basedir of the watch paths to accomplish the goal of this PR.

@laggingreflex
Copy link
Author

You're right my bad. It seems it already works the way you described, I just needed to add cwd option.

@laggingreflex
Copy link
Author

So just to clarify, it only excludes either when both the watch path and exclude paths are either both relative or when both are absolute. I had a watch path as an absolute path and ignored paths as relative paths. So in this case it is required to have cwd option passed in for it to work, right?

Would it be desirable to have it auto infer cwd if the watch path is absolute? Or is this intended?

@es128
Copy link
Contributor

es128 commented Oct 10, 2016

Better to think in terms of how globs are matched. The pattern path/to/*.js does not match on /some/path/to/file.js unless you first convert one or the other to be treated as absolute or relative from the same base.

Would it be desirable to have it auto infer cwd if the watch path is absolute? Or is this intended?

This is the same as the suggestion I made at the end of my comment. Might be worth it, but there might also be a risk of side effects.

In your case using cwd is one viable solution. The other is to keep the inputs consistent with each other.

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

Successfully merging this pull request may close these issues.

3 participants