laughinghan/choreographer

** Matching issue

technofailure opened this issue · 4 comments

technofailure commented

The ** matching isn't working the way you want it to, the problem is:

          String(route)
            .replace(specialChars, '\\$&')
            .replace(/\*\*/g, '(.*)')
            .replace(/\*/g, '([^/]*)')

You replace ** with (.*), which itself contains a *. You then search for single * and replace it further.

So, '**' is first transformed to '(.)', which is then transformed to '(.([^/]))'. This obviously just matches a single any character (the dot) followed my 0 or more non forward slashes, and not any character, including slashes. What you could do instead would be to

          String(route)
            .replace(specialChars, '\\$&')
            .replace(/\*\*/g, '++')
            .replace(/\*/g, '([^/]*)')
            .replace(/\+\+/g, '(.*)') +

This would replace ** with ++, then find any single 's, then change the ++ to the intended (.). Since the first ".replace(specialChars, '$&')" escapes all regularly occuring +'s, the last replace will not get a false positive.

There are obviously other ways you could fix the issue, but this was the first that came to me.

gsf commented

I didn't see this issue and just sent in a pull request for the same thing (see #11). I like your fix, as it maintains the current behavior. Mine is less clever.

gsf commented

Would you mind wrapping this fix into a new release? The latest I'm seeing via npm is 0.2.3a. Would a pull request for this help that along?

gsf commented

Oh and one more thought about my pull request at #11. While a .+ regex rather than a .* does change the behavior, it's actually more in line with command line globbing, which is what I think of when I'm using these routes. For example:

$ echo git/
git/
$ echo git/*
git/checky git/choreographer git/knowtify git/sheltr
$

Notice that "git/" isn't included in the match for "git/*".

laughinghan commented

@gsf But if you have a bunch of files pre, preThing, preOtherThing, pre* would match all of them, including just plain pre. I think * mapping to .+ is a special Bash or Unix exception for directories.

With Sinatra, for example, * appears to always mean .*, so I think we should stick with that.