search

damnhandy / Handy-URI-Templates

A Java URI Template processor implementing RFC6570
https://damnhandy.github.io/Handy-URI-Templates/
Other
203 stars 37 forks source link

Fix for building URI templates vars w/ underscores #38

Closed petejohanson closed 8 years ago

petejohanson commented 9 years ago
petejohanson commented 9 years ago

The reason this bombs in Expression#buildMatchingPattern() is because it attempts to use the variable name as the name of the named capture group, and "_" is not a valid character for a named capture group, so it throws.

By relying on lazy initialization in Expression#getMatchPattern(), we avoid hitting that problem for the simple builder case.

damnhandy commented 8 years ago

Thanks @petejohanson! Although, I'm curious as to why this hasn't come up when the tests are run. The uritemplatetest suite has names with underscores in it already and those are passing. I'm going to dig deeper before I accept.

petejohanson commented 8 years ago

Does the test suite use the builder at all? Or just parsing of existing Templates then processing? On Nov 12, 2015 6:47 PM, "Ryan J. McDonough" notifications@github.com wrote:Thanks @petejohanson! Although, I'm curious as to why this hasn't come up when the tests are run. The uritemplatetest suite has names with underscores in it already and those are passing. I'm going to dig deeper before I accept.

—Reply to this email directly or view it on GitHub.

petejohanson commented 8 years ago

https://github.com/damnhandy/Handy-URI-Templates/blob/master/src/test/java/com/damnhandy/uri/template/conformance/AbstractUriTemplateConformanceTest.java doesn't seem to use the builder at all, just parsing from existing template. Different code path?

damnhandy commented 8 years ago

I missed that part about it being related to the URI Template Builder. The conformance tests just focus on the template expansion. You fixe is a completely different code path. Accepting the PR. Thanks!

petejohanson commented 8 years ago

Any idea when you'll be doing the next release that includes this fix? Gauging whether I need to switch to -SNAPSHOT temporarily or not.

Thanks.

damnhandy commented 8 years ago

Just did :)

https://github.com/damnhandy/Handy-URI-Templates/releases/tag/handy-uri-templates-2.1.1

Should be bubbling up to Maven Central now.

petejohanson commented 8 years ago

Perfect, thanks for the quick turn around!