Fix handling of optionals in the middle

[paulmelnikow]

Thanks so much for this library! It’s come in very handy in Shields. We’ve significantly reduced the duplication in our route definitions, and we’re getting really close letting developers build badge URLs in the frontend.

We’re running into an issue when an optional token precedes the extension. Many of the routes look something like this:

/travis/:user/:repo/:branch?.ext(svg|png)

• That pattern matches /travis/foo/bar/master.svg as we’d expect
• Unexpectedly it does not match /travis/foo/bar.svg
• Unexpectedly it does match /travis/foo/bar/.svg

I traced the behavior back to #71 where it was introduced. I think it was unintentional since that PR was about making the leading delimiter optional for an optional token. I don’t see why the delimiter should be present before an optional in the middle.

It’s kind of an odd fix, though it makes some sense given #71 was about fixing leading optionals.

[coveralls]

Coverage remained the same at 100.0% when pulling fd29ee3 on paulmelnikow:fix-mid-optionals into c36498d on pillarjs:master.

[blakeembrey]

@paulmelnikow If this is the wanted behavior, I'm actually leaning toward partial being a bad solution here. I might actually remove it altogether, since that's effectively what this PR is doing outside of the first token position. In that case, both are a large breaking change, but at least removing the behavior everywhere should make things more consistent. Plus, there's an easy workaround for when you do want this - use the backslash to escape the / from being interpreted.

I'm going to do a follow up PR based on this to explore, hope that's ok 😄

[paulmelnikow]

Hi!

I'm going to do a follow up PR based on this to explore, hope that's ok

Go for it 👍

I see why dropping partial would be a breaking change, though the separator being required between the optional and required token seems like a bug. (It's not under test, of course.)

Plus, there's an easy workaround for when you do want this - use the backslash to escape the / from being interpreted.

A workaround might be fine, though I'm not sure I understand what you're suggesting. Could you clarify?

[blakeembrey]

A workaround might be fine, though I'm not sure I understand what you're suggesting. Could you clarify?

I'm sorry, I'm terrible at explaining this when it's all going through my head and all theoretical. I meant that if we remove partial, it'd be possible to get the old behavior back easily by escaping the delimiter. I don't think the workaround is for today's version.

I'm playing around with it to test, the only odd thing in the tests is /:test* support with delimiter. I think I may need to add a different token to say repeat and match first prefix. It's probable that this is a just an unused edge case too, but wanted to get your take on it since you opened the suggestion. What should /:test*-ext do?

[blakeembrey]

Feel free to take a look #176 for an example.

[paulmelnikow]

What should /:test*-ext do?

Shields may be an edge case, though I can say that we're adapting long established regex-based URL patterns to the library. So in that sense it's not contrived: these are the URLs we need to continue to support, at least for the moment.

Here's an example with *:

/swagger/valid/2.0/:scheme(http|https)?/:url*.:ext(svg|png|gif|jpg|json)

Path I would like to match:

/swagger/valid/2.0/https/raw.githubusercontent.com/OAI/OpenAPI-Specification/master/examples/v2.0/json/petstore-expanded.json.svg

Added: As I digest #176 and think about this more, perhaps this is the correct pattern:

/swagger/valid/2.0/:scheme(http|https)?/:url+.:ext(svg|png|gif|jpg|json)

[paulmelnikow]

I'll mention that from perspective of working with this in Shields, escaping the backslash before a leading optional but not before a leading required seems less convenient or intuitive, compared to never escaping it as in this PR. We assemble the pattern by concatenating these:

1. Fixed /
2. Per-route base like swagger/valid/2.0
3. Per-route pattern like :scheme(http|https)?/:url+
4. Fixed .:ext(svg|png|gif|jpg|json)

Put another way: a route needs to specify base and pattern, but the leading and connecting / are implicit and so is the trailing extension.

The reason we make a division between base and pattern is to avoid duplication: the base becomes implicit in our tests. In a couple rare cases out of necessity the base is empty.

In a couple others, the pattern is empty because the route is literal all the way up to the extension. However that could be expressed adequately (though slightly less conveniently, for the tests) with an empty base and a literal pattern.

I hope I'm sharing this in a way that's helpful. Again this is not in my head as much as it's in yours and my perspective is from 10,000 feet away.

[blakeembrey]

@paulmelnikow Thanks! I think it makes sense. I mostly don't want to mess with /:test? meaning different things in different positions, it'll be a little confusing. And based on this, it does sounds like you really want /:url+ (this will make sure you have at least one segment).

[paulmelnikow]

If you'd like, I'd be happy to try to integrate Shields with the code from #176 to see if the cases we want to support are working.

[blakeembrey]

@paulmelnikow Sure, that'd actually be helpful. The biggest concern I have is that I removed delimiters being explicitly configured to support #168, but don't let that change block you. Let me know if it breaks anything.