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

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@brycemcd
Contributor

brycemcd commented Mar 9, 2012

Is useful. Solves for Issue #49

@jeremy

This comment has been minimized.

Show comment Hide comment
@jeremy

jeremy Mar 9, 2012

Member

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

Member

jeremy commented Mar 9, 2012

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

@brycemcd

This comment has been minimized.

Show comment Hide comment
@brycemcd

brycemcd Mar 9, 2012

Contributor

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

Contributor

brycemcd commented Mar 9, 2012

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

This comment has been minimized.

Show comment Hide comment
@jeremy

jeremy Mar 9, 2012

Member

@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.

Member

jeremy commented Mar 9, 2012

@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

This comment has been minimized.

Show comment Hide comment
@brycemcd

brycemcd Mar 10, 2012

Contributor

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

Contributor

brycemcd commented Mar 10, 2012

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

@jeremy

This comment has been minimized.

Show comment Hide comment
@jeremy

jeremy Mar 10, 2012

Member

Well that's a leap! ;)

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

Member

jeremy commented Mar 10, 2012

Well that's a leap! ;)

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

@rkh

This comment has been minimized.

Show comment Hide comment
@rkh

rkh Dec 10, 2012

Member

@brycem will merge if you rebase onto master.

Member

rkh commented Dec 10, 2012

@brycem will merge if you rebase onto master.

@brycemcd

This comment has been minimized.

Show comment Hide comment
@brycemcd

brycemcd Dec 12, 2012

Contributor

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

Contributor

brycemcd commented Dec 12, 2012

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

@mpalmer

This comment has been minimized.

Show comment Hide comment
@mpalmer

mpalmer Oct 31, 2014

Contributor

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.
Contributor

mpalmer commented Oct 31, 2014

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.
@jjb

This comment has been minimized.

Show comment Hide comment
@jjb

jjb Nov 2, 2014

Contributor

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.

Contributor

jjb commented Nov 2, 2014

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

This comment has been minimized.

Show comment Hide comment
@brycemcd

brycemcd Nov 3, 2014

Contributor

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.

Contributor

brycemcd commented Nov 3, 2014

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.

@mpalmer mpalmer added needs-docs and removed needs-cleanup labels Jun 22, 2015

@mpalmer mpalmer self-assigned this Jun 22, 2015

@mpalmer

This comment has been minimized.

Show comment Hide comment
@mpalmer

mpalmer Jun 22, 2015

Contributor

I'll do the needful on this PR, and get it ready for release.

Contributor

mpalmer commented Jun 22, 2015

I'll do the needful on this PR, and get it ready for release.

@jjb

This comment has been minimized.

Show comment Hide comment
@jjb

jjb Jun 23, 2015

Contributor

While you're add it, might be nice to have a line of documentation for this feature in the comments.

Contributor

jjb commented Jun 23, 2015

While you're add it, might be nice to have a line of documentation for this feature in the comments.

@jjb

This comment has been minimized.

Show comment Hide comment
@jjb

jjb Jun 23, 2015

Contributor

Also: @mperham -- any news thoughts on this since your request (#49) in 2012?

Contributor

jjb commented Jun 23, 2015

Also: @mperham -- any news thoughts on this since your request (#49) in 2012?

@mpalmer

This comment has been minimized.

Show comment Hide comment
@mpalmer

mpalmer Jun 23, 2015

Contributor

@jjb hence the needs-docs tag. (grin)

Contributor

mpalmer commented Jun 23, 2015

@jjb hence the needs-docs tag. (grin)

@jjb

This comment has been minimized.

Show comment Hide comment
@jjb

jjb Jun 23, 2015

Contributor

Ah, excellent :)

Contributor

jjb commented Jun 23, 2015

Ah, excellent :)

@brycemcd

This comment has been minimized.

Show comment Hide comment
@brycemcd

brycemcd Jun 23, 2015

Contributor

@mpalmer looks like interest in this has really picked up. I'm happy to jump back in and resubmit (using your overview on Oct. 31, 2104) and do the doc update if you'd like. Let me know either way.

The only reason I didn't resubmit before was because I didn't think there was any interest

Contributor

brycemcd commented Jun 23, 2015

@mpalmer looks like interest in this has really picked up. I'm happy to jump back in and resubmit (using your overview on Oct. 31, 2104) and do the doc update if you'd like. Let me know either way.

The only reason I didn't resubmit before was because I didn't think there was any interest

@mpalmer

This comment has been minimized.

Show comment Hide comment
@mpalmer

mpalmer Jun 23, 2015

Contributor

@brycemcd, yes rack-contrib was moribund for a while, but a group of us are working to bring it back to life. If you could put together some docs and cleanup, that would be very helpful, as it would leave me free to put the polish on other PRs. We're doing monthly feature releases (first-of-the-month or so), so whenever you're able to get it sorted, it'll be released shortly thereafter.

Contributor

mpalmer commented Jun 23, 2015

@brycemcd, yes rack-contrib was moribund for a while, but a group of us are working to bring it back to life. If you could put together some docs and cleanup, that would be very helpful, as it would leave me free to put the polish on other PRs. We're doing monthly feature releases (first-of-the-month or so), so whenever you're able to get it sorted, it'll be released shortly thereafter.

@brycemcd

This comment has been minimized.

Show comment Hide comment
@brycemcd

brycemcd Jun 24, 2015

Contributor

@mpalmer happy to help out. I have another PR I'm working on at the moment, but I just set aside some time next week to get this taken care of.

Contributor

brycemcd commented Jun 24, 2015

@mpalmer happy to help out. I have another PR I'm working on at the moment, but I just set aside some time next week to get this taken care of.

@mpalmer

This comment has been minimized.

Show comment Hide comment
@mpalmer

mpalmer Jul 20, 2015

Contributor

I've created #111 as a cleaned-up version of this PR. Closing this one.

Contributor

mpalmer commented Jul 20, 2015

I've created #111 as a cleaned-up version of this PR. Closing this one.

@mpalmer mpalmer closed this Jul 20, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment