-
Notifications
You must be signed in to change notification settings - Fork 36
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
Support rack 2.x #320
Support rack 2.x #320
Conversation
Signed-off-by: Josep M. Blanquer <blanquer@gmail.com>
lib/praxis/multipart/parser.rb
Outdated
@@ -87,9 +88,13 @@ def setup_parse | |||
|
|||
@buf = "" | |||
|
|||
@params = Rack::Utils::KeySpaceConstrainedParams.new | |||
@params = if rack_2x? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could make this method definition itself conditional based on the logic below. It won't change at runtime, and shaving off the check couldn't hurt
Signed-off-by: Josep M. Blanquer <blanquer@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@thogg4 does this look good to you as well? since you were the one who initially looked at it? |
@@ -20,7 +20,7 @@ Gem::Specification.new do |spec| | |||
spec.bindir = 'bin' | |||
spec.executables << 'praxis' | |||
|
|||
spec.add_dependency 'rack', '~> 1' | |||
spec.add_dependency 'rack', '>= 1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does is make sense to make this >= 1 < 3
so that this doesn't break with Rack 3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't foresee any rack 3 happening (just released version two now...and rack exists since 2007)...plus it is a low-level framework to major numbers are a big deal to release...so I don't think we should worry about it. IMO at least.
note: @thogg4 regarding your PR #319 you had some of the basics in there but it was not properly done. Thank you very much for contributing though! it made it much easier to delve into it. See my comments in the PR.
Signed-off-by: Josep M. Blanquer blanquer@gmail.com