webmachine / webmachine-ruby

Webmachine, the HTTP toolkit (in Ruby)
http://rdoc.info/github/seancribbs/webmachine-ruby/master/frames
Other
852 stars 54 forks source link

Added support for regex in path spec. #235

Closed rfestag closed 8 years ago

rfestag commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

rgarner commented 8 years ago

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

Asmod4n commented 8 years ago

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

rfestag commented 8 years ago

Done, and it looks like all checks passed.

Asmod4n commented 8 years ago

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