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

Added support for regex in path spec. #235

Merged
merged 2 commits into from
Apr 25, 2016

Conversation

rfestag
Copy link
Contributor

@rfestag rfestag commented Apr 23, 2016

Supports adding :captures path_info element for any capture groups.

I ran the following quick benchmark to verify that traditional (string/symbol) routes are not impacted significantly. Basically, we check whether the spec is a regex (like how Symbols work), and only then incur the overhead of regex matching and pulling out capture groups.

require 'webmachine'
require 'benchmark'

request = Webmachine::Request.new("GET", URI.parse("http://localhost:8080/hello/bob.json"), Webmachine::Headers["accept" => "*/*"], "")
route1 =  Webmachine::Dispatcher::Route.new ['hello', :string], Class.new(Webmachine::Resource), {}
route2 =  Webmachine::Dispatcher::Route.new ['hello', /(.*)/], Class.new(Webmachine::Resource), {}

n = 50000
Benchmark.bm do |x|
  x.report('Traditional') {n.times {route1.match? request}}
  x.report('Regex') {n.times {route2.match? request}}
end

@seancribbs
Copy link
Member

I like this better than your other suggestion, especially since it's more flexible than explicitly recognizing "file extensions" as a thing. Let's see if Travis will build it correctly before we merge.

Other contributors, please weigh in!

@rgarner
Copy link
Contributor

rgarner commented Apr 23, 2016

My main weigh-in won't be directly related to this PR, but rather to the build-tidiness PR. Rubinius is usually the ruby that's making our builds fail there (though really, we need to extract the adapters to gems). I lack the time to track down exactly why this is, because we've got multiple adapters failing for subtly different reasons on multiple rubies with slightly different threading behaviours, especially around Fibers; I'd suggest we move Rubinius to the "allowed failures" list there for the time being, because it will continue to block passing checks on useful PRs like this one.

@rfestag
Copy link
Contributor Author

rfestag commented Apr 23, 2016

I haven't done much with Travis, but it looks like it is failing during the bundle install install (something about spec being nil?). I was having issues running all specs locally (which fails anywhere from 13 to 90 tests, I'm assuming because of timeouts). That is with Ruby 2.3.0.

Was that fixed in the build-tidiness PR? Or is there something I need to fix on my end to at least get the tests to run to the same point I'm seeing locally?

@Asmod4n
Copy link
Member

Asmod4n commented Apr 23, 2016

👍 on removing all adapters except those bundled with ruby itself.

@rgarner
Copy link
Contributor

rgarner commented Apr 24, 2016

@rfestag, yes, that was fixed in 07f02c2, which is part of #233. A known problem with some Travis rubies.

@Asmod4n
Copy link
Member

Asmod4n commented Apr 24, 2016

@rfestag could you merge in the changes since the last commit?

@rfestag
Copy link
Contributor Author

rfestag commented Apr 24, 2016

Done, and it looks like all checks passed.

@Asmod4n
Copy link
Member

Asmod4n commented Apr 24, 2016

If no-one has objections ill merge it in the next 48 hours.

@Asmod4n Asmod4n merged commit 7a22afc into webmachine:master Apr 25, 2016
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.

4 participants