Skip to content
This repository has been archived by the owner on Jul 13, 2023. It is now read-only.

Interpolate URLs using a string rather than a regex #1940

Closed
wants to merge 3 commits into from

Conversation

stretchkennedy
Copy link
Contributor

Several other users have had problems with the speed of Paperclip::Interpolations.interpolate or with model.attachment.url (#557, #909). gsub with a string is significantly faster than gsub with a regex, and changing to a string also prevents users from creating interpolations with ? wildcards in them.

See:

irb(main):001:0> require 'benchmark'
=> true
irb(main):002:0> puts Benchmark.measure{100_000.times{":foo/:bar".gsub(/:#{:bar}/, 'BAR')}}
  2.290000   0.030000   2.320000 (  2.425150)
=> nil
irb(main):003:0> puts Benchmark.measure{100_000.times{":foo/:bar".gsub(":#{:bar}", 'BAR')}}
  0.860000   0.010000   0.870000 (  1.001322)
=> nil

@@ -30,7 +30,7 @@ def self.all
def self.interpolate pattern, *args
pattern = args.first.instance.send(pattern) if pattern.kind_of? Symbol
all.reverse.inject(pattern) do |result, tag|
result.gsub(/:#{tag}/) do |match|
result.gsub(":#{tag}") do |match|

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused block argument - match. You can omit the argument if you don't care about it.

@dgynn
Copy link
Contributor

dgynn commented Jul 18, 2015

You might be interested in #1888 which also does gsub against a string token as well as some other performance optimizations. I've tried you test case with a :foo? interpolator and that works too.

@stretchkennedy
Copy link
Contributor Author

I totally missed #1888. I'm fine with closing this PR, but it would be great to get the test case into the other one.

@jyurek
Copy link

jyurek commented Aug 21, 2015

Ok, then. Seeing as this is a subset of what #1888 provides, I'll close this. Thanks for submitting it, though, @stretchkennedy.

@jyurek jyurek closed this Aug 21, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants