-
Notifications
You must be signed in to change notification settings - Fork 556
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
Optimizes normalize_headers
for performance
#1029
Optimizes normalize_headers
for performance
#1029
Conversation
Optimizes the `WebMock::Util::Headers.normalize_headers` method to: * Create less objects * Avoid using Regexp where possible * Hoists constants More details can be found in the related issue, including performance benchmarks: bblimke#1027
99924c2
to
1249b51
Compare
[name.to_s.split(/_|-/).map { |segment| segment.capitalize }.join("-"), | ||
case value | ||
|
||
headers.each_with_object({}) do |(name, value), new_headers| |
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'm not a maintainer, but each_with_object
is an ActiveSupport method, and webmock doesn't depend on ActiveSupport. You'll probably need to change this for the test suite to pass.
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.
https://ruby-doc.org/3.2.2/Enumerable.html#method-i-each_with_object
Enumerable#each_with_object
is in Ruby itself dating back several versions, confirmed that it's supported back to Ruby 1.9.3.
Also test suite was passing locally with this modification.
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.
Wow. TIL. Thanks for the clarification!
Is there anything that can be done to help this along? I've been profiling our app's system specs (same one as @baweaver), and this is one I keep coming back to. |
@@ -57,6 +60,15 @@ def self.basic_auth_header(*credentials) | |||
"Basic #{Base64.strict_encode64(credentials.join(':')).chomp}" | |||
end | |||
|
|||
def self.normalize_name(name) |
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.
This code is very easy to read now. Thank you @baweaver !
Thank you for merging 🙇🏻 Any chance we can get a release with it and #1033 🙏🏻 |
@technicalpickles 3.19.0 is now released. thank you! |
Optimizes the
WebMock::Util::Headers.normalize_headers
method to:More details can be found in the related issue, including performance benchmarks:
#1027