Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

added 'times' to be called in the query string #52

Open
wants to merge 1 commit into from

5 participants

@brycemcd

Is useful. Solves for Issue #49

@jeremy
Owner

This sets @times for all requests, though. How about setting it just for the current request?

@brycemcd

it should set @times for just the request here https://github.com/rack/rack-contrib/pull/52/files#L0R30 without overwriting the more global times setting

@jeremy
Owner

@brycem a single middleware instance is used for all requests, so setting an instance variable here means the @times setting will apply to every subsequent request.

@brycemcd

so there's really no way to solve Issue #49?

@jeremy
Owner

Well that's a leap! ;)

You could pass times as a local variable instead of setting the instance variable.

@rkh
Owner

@brycem will merge if you rebase onto master.

@brycemcd

yikes, it's been 9 months?! I'll add times as a local variable and then rebase

@mpalmer
Collaborator

I really like the concept of this PR. I've got a few concerns, however:

  1. Definitely should be overriding the instance variable;
  2. There's no reason for request to have been changed to @request, as far as I can see;
  3. times as a query param seems a little too... generic, and likely to be accidentally tripped over. Something more unique, like profiler_times, would be safer;
  4. The gemspec should remain untouched.
@mpalmer mpalmer added the needs-work label
@jjb
Collaborator
jjb commented

this PR has a lot of problems and @brycemcd hasn't responded to feedback. also the included test is weak, uses instance_variable_get. i vote for closing this.

@brycemcd

I forgot this was even open. I haven't looked at it in ~ 2 years. If it's really something that would be useful (to more than just @mpalmer ;) ) I can clean it up and re-submit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 9, 2012
  1. @brycemcd
This page is out of date. Refresh to see the latest.
View
5 lib/rack/contrib/profiler.rb
@@ -27,6 +27,7 @@ def initialize(app, options = {})
def call(env)
if mode = profiling?(env)
+ @times = @request.params['times'].to_i if @request.params['times']
profile(env, mode)
else
@app.call(env)
@@ -36,8 +37,8 @@ def call(env)
private
def profiling?(env)
unless ::RubyProf.running?
- request = Rack::Request.new(env.clone)
- if mode = request.params.delete('profile')
+ @request = Rack::Request.new(env.clone)
+ if mode = @request.params.delete('profile')
if ::RubyProf.const_defined?(mode.upcase)
mode
else
View
2  rack-contrib.gemspec
@@ -4,7 +4,7 @@ Gem::Specification.new do |s|
s.name = 'rack-contrib'
s.version = '1.1.0'
- s.date = '2010-10-19'
+ s.date = '2012-03-08'
s.description = "Contributed Rack Middleware and Utilities"
s.summary = "Contributed Rack Middleware and Utilities"
View
5 test/spec_rack_profiler.rb
@@ -14,6 +14,11 @@
profiler.instance_variable_get('@times').should.equal 1
end
+ specify 'call @times globally and times is set' do
+ profiler = Rack::Profiler.new(app, :times => 4)
+ profiler.instance_variable_get('@times').should.equal 4
+ end
+
specify 'CallStackPrinter has Content-Type test/html' do
headers = Rack::Profiler.new(app, :printer => :call_stack).call(request)[1]
headers.should.equal "Content-Type"=>"text/html"
Something went wrong with that request. Please try again.