-
Notifications
You must be signed in to change notification settings - Fork 21.9k
Allow Rack::Runtime to be deleted from middleware stack. #18994
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
Allow Rack::Runtime to be deleted from middleware stack. #18994
Conversation
@@ -64,9 +65,12 @@ def unshift(*args, &block) | |||
end | |||
|
|||
def merge_into(other) #:nodoc: | |||
@operations.each do |operation, args, block| | |||
other.send(operation, *args, &block) | |||
[@operations, @delete_operations].each do |operations| |
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.
Instead of two loops can we just concat the @opeartions
with @delete_operations
?
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.
@rafaelfranca That is a good point 👍 Thank you!
9461aff
to
a39498a
Compare
@rafaelfranca Updated |
…ons_last Allow Rack::Runtime to be deleted from middleware stack.
@@ -64,9 +65,10 @@ def unshift(*args, &block) | |||
end | |||
|
|||
def merge_into(other) #:nodoc: | |||
@operations.each do |operation, args, block| | |||
@operations.concat(@delete_operations).each do |operation, args, block| |
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.
Should we be actually changing the operations array? Maybe we should create another array by adding both?
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 think changing the operations array is fine since delete_operations are meant to be part of the initial array just that we're just appending the delete operations last to run them last.
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.
Right. I think we only call this once, which is probably ok this way, but what would happen if we call it more times?
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.
Ah ok. So the use case is when merge_into has been called and we call it again which means new operations are appended after delete operation?
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 think to address the concerns we could just either create a new array and just loop through both type of operations.
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.
Fixed at 6ad28a7
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.
❤️
On Thu, Feb 19, 2015 at 9:33 AM, Rafael Mendonça França <
notifications@github.com> wrote:
In railties/lib/rails/configuration.rb
#18994 (comment):@@ -64,9 +65,10 @@ def unshift(*args, &block)
enddef merge_into(other) #:nodoc:
@operations.each do |operation, args, block|
@operations.concat(@delete_operations).each do |operation, args, block|
—
Reply to this email directly or view it on GitHub
https://github.com/rails/rails/pull/18994/files#r24981139.
At.
Carlos Antonio
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.
Thanks @rafaelfranca 😄
Fixes: #16433.