Skip to content
This repository has been archived by the owner on Aug 5, 2022. It is now read-only.

Match path if regex starts with / #39

Closed
wants to merge 3 commits into from

Conversation

mishak87
Copy link

Pull to #36 issue.
On Windows replace of / to \\\\ is done before matching.

@execjosh
Copy link
Owner

Wow, thanks! I won't be able to check this for a few more days; but, thank you very much for helping out 😄 Hold on a few more days!

@execjosh execjosh added this to the v0.6.0 milestone Mar 19, 2016
@execjosh
Copy link
Owner

I'd like to get some specs working before I merge this in so we can be sure nothing breaks.

@execjosh
Copy link
Owner

Oh, wait... There already are some specs for ScopeNameProvider. brain fart

@execjosh
Copy link
Owner

Okay, I've read through your code. I'm not sure I like requiring that path matchers begin with slashes. I'll think about this.

@execjosh
Copy link
Owner

Oh, and please add tests to cover the new use cases

@mishak87
Copy link
Author

IMO using Regex in config (not in implementation) for path matching is overkill. Utilizing bash glob or go filepath matching syntax would be better since we are dealing with files and more people are already use that syntax to match files. Also slash at the beginning would not be needed.

I have added slash to avoid possible false positives with configs written for old versions ie.:

'r.*b.ext': 'source.ruby' # user used path unaware config
# would match: r/b.ext
# not just:    rib.ext

Will add tests ASAP (end of this week at latest).

@execjosh
Copy link
Owner

I'm open to the idea of bash globbing. As for v0.5.x, though, RegExp will stay.

BTW, I just implemented the longest-match and tie-breaker logic, as well as deprecating extension matchers. Those changes conflict with this PR. Sorry for totally breaking your work effort...

@execjosh
Copy link
Owner

I've opened up #49 to talk about bash globbing 👍

@execjosh execjosh removed this from the v0.6.0 milestone Jul 23, 2016
@execjosh
Copy link
Owner

I'm going to close this for now in favor of #51.

@execjosh execjosh closed this Jul 24, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants