-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Cache interpolator methods and reduce memory allocations - rebased on v4.3 #2056
Changes from all commits
3def1e1
993a47b
18e1c5a
5584734
73865db
07b8b9e
48cad66
dfb1542
6eb2093
9ce7f22
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ module Interpolations | |
# and is not intended for normal use. | ||
def self.[]= name, block | ||
define_method(name, &block) | ||
@interpolators_cache = nil | ||
end | ||
|
||
# Hash access of interpolations. Included only for compatibility, | ||
|
@@ -20,7 +21,7 @@ def self.[] name | |
|
||
# Returns a sorted list of all interpolations. | ||
def self.all | ||
self.instance_methods(false).sort | ||
self.instance_methods(false).sort! | ||
end | ||
|
||
# Perform the actual interpolation. Takes the pattern to interpolate | ||
|
@@ -29,11 +30,15 @@ def self.all | |
# an interpolation pattern for Paperclip to use. | ||
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| | ||
send( tag, *args ) | ||
end | ||
result = pattern.dup | ||
interpolators_cache.each do |method, token| | ||
result.gsub!(token) { send(method, *args) } if result.include?(token) | ||
end | ||
result | ||
end | ||
|
||
def self.interpolators_cache | ||
@interpolators_cache ||= all.reverse!.map! { |method| [method, ":#{method}"] } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line is too long. [84/80] |
||
end | ||
|
||
def self.plural_cache | ||
|
@@ -42,7 +47,7 @@ def self.plural_cache | |
|
||
# Returns the filename, the same way as ":basename.:extension" would. | ||
def filename attachment, style_name | ||
[ basename(attachment, style_name), extension(attachment, style_name) ].reject(&:blank?).join(".") | ||
[ basename(attachment, style_name), extension(attachment, style_name) ].delete_if(&:empty?).join(".".freeze) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line is too long. [114/80] |
||
end | ||
|
||
# Returns the interpolated URL. Will raise an error if the url itself | ||
|
@@ -85,28 +90,28 @@ def rails_env attachment, style_name | |
# all class names. Calling #class will return the expected class. | ||
def class attachment = nil, style_name = nil | ||
return super() if attachment.nil? && style_name.nil? | ||
plural_cache.underscore_and_pluralize(attachment.instance.class.to_s) | ||
plural_cache.underscore_and_pluralize_class(attachment.instance.class) | ||
end | ||
|
||
# Returns the basename of the file. e.g. "file" for "file.jpg" | ||
def basename attachment, style_name | ||
attachment.original_filename.gsub(/#{Regexp.escape(File.extname(attachment.original_filename))}\Z/, "") | ||
File.basename(attachment.original_filename, ".*".freeze) | ||
end | ||
|
||
# Returns the extension of the file. e.g. "jpg" for "file.jpg" | ||
# If the style has a format defined, it will return the format instead | ||
# of the actual extension. | ||
def extension attachment, style_name | ||
((style = attachment.styles[style_name.to_s.to_sym]) && style[:format]) || | ||
File.extname(attachment.original_filename).gsub(/\A\.+/, "") | ||
File.extname(attachment.original_filename).sub(/\A\.+/, "".freeze) | ||
end | ||
|
||
# Returns the dot+extension of the file. e.g. ".jpg" for "file.jpg" | ||
# If the style has a format defined, it will return the format instead | ||
# of the actual extension. If the extension is empty, no dot is added. | ||
def dotextension attachment, style_name | ||
ext = extension(attachment, style_name) | ||
ext.empty? ? "" : ".#{ext}" | ||
ext.empty? ? ext : ".#{ext}" | ||
end | ||
|
||
# Returns an extension based on the content type. e.g. "jpeg" for | ||
|
@@ -170,9 +175,9 @@ def hash attachment=nil, style_name=nil | |
def id_partition attachment, style_name | ||
case id = attachment.instance.id | ||
when Integer | ||
("%09d" % id).scan(/\d{3}/).join("/") | ||
("%09d".freeze % id).scan(/\d{3}/).join("/".freeze) | ||
when String | ||
id.scan(/.{3}/).first(3).join("/") | ||
id.scan(/.{3}/).first(3).join("/".freeze) | ||
else | ||
nil | ||
end | ||
|
@@ -181,7 +186,7 @@ def id_partition attachment, style_name | |
# Returns the pluralized form of the attachment name. e.g. | ||
# "avatars" for an attachment of :avatar | ||
def attachment attachment, style_name | ||
plural_cache.pluralize(attachment.name.to_s.downcase) | ||
plural_cache.pluralize_symbol(attachment.name) | ||
end | ||
|
||
# Returns the style, or the default style if nil is supplied. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -141,7 +141,7 @@ def sanitize_hash(hash) | |
Proc.new do |style, attachment| | ||
permission = (@s3_permissions[style.to_s.to_sym] || @s3_permissions[:default]) | ||
permission = permission.call(attachment, style) if permission.respond_to?(:call) | ||
(permission == :public_read) ? 'http' : 'https' | ||
(permission == :public_read) ? 'http'.freeze : 'https'.freeze | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping. |
||
end | ||
@s3_metadata = @options[:s3_metadata] || {} | ||
@s3_headers = {} | ||
|
@@ -157,26 +157,26 @@ def sanitize_hash(hash) | |
@s3_server_side_encryption = @options[:s3_server_side_encryption] | ||
end | ||
|
||
unless @options[:url].to_s.match(/\A:s3.*url\Z/) || @options[:url] == ":asset_host" | ||
@options[:path] = path_option.gsub(/:url/, @options[:url]).gsub(/\A:rails_root\/public\/system/, '') | ||
@options[:url] = ":s3_path_url" | ||
unless @options[:url].to_s.match(/\A:s3.*url\Z/) || @options[:url] == ":asset_host".freeze | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line is too long. [100/80] |
||
@options[:path] = path_option.gsub(/:url/, @options[:url]).sub(/\A:rails_root\/public\/system/, "".freeze) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line is too long. [118/80] |
||
@options[:url] = ":s3_path_url".freeze | ||
end | ||
@options[:url] = @options[:url].inspect if @options[:url].is_a?(Symbol) | ||
|
||
@http_proxy = @options[:http_proxy] || nil | ||
end | ||
|
||
Paperclip.interpolates(:s3_alias_url) do |attachment, style| | ||
"#{attachment.s3_protocol(style, true)}//#{attachment.s3_host_alias}/#{attachment.path(style).gsub(%r{\A/}, "")}" | ||
"#{attachment.s3_protocol(style, true)}//#{attachment.s3_host_alias}/#{attachment.path(style).sub(%r{\A/}, "".freeze)}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line is too long. [129/80] |
||
end unless Paperclip::Interpolations.respond_to? :s3_alias_url | ||
Paperclip.interpolates(:s3_path_url) do |attachment, style| | ||
"#{attachment.s3_protocol(style, true)}//#{attachment.s3_host_name}/#{attachment.bucket_name}/#{attachment.path(style).gsub(%r{\A/}, "")}" | ||
"#{attachment.s3_protocol(style, true)}//#{attachment.s3_host_name}/#{attachment.bucket_name}/#{attachment.path(style).sub(%r{\A/}, "".freeze)}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line is too long. [154/80] |
||
end unless Paperclip::Interpolations.respond_to? :s3_path_url | ||
Paperclip.interpolates(:s3_domain_url) do |attachment, style| | ||
"#{attachment.s3_protocol(style, true)}//#{attachment.bucket_name}.#{attachment.s3_host_name}/#{attachment.path(style).gsub(%r{\A/}, "")}" | ||
"#{attachment.s3_protocol(style, true)}//#{attachment.bucket_name}.#{attachment.s3_host_name}/#{attachment.path(style).sub(%r{\A/}, "".freeze)}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line is too long. [154/80] |
||
end unless Paperclip::Interpolations.respond_to? :s3_domain_url | ||
Paperclip.interpolates(:asset_host) do |attachment, style| | ||
"#{attachment.path(style).gsub(%r{\A/}, "")}" | ||
"#{attachment.path(style).sub(%r{\A/}, "".freeze)}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer single-quoted strings inside interpolations. |
||
end unless Paperclip::Interpolations.respond_to? :asset_host | ||
end | ||
|
||
|
@@ -197,7 +197,7 @@ def s3_host_name | |
host_name = @options[:s3_host_name] | ||
host_name = host_name.call(self) if host_name.is_a?(Proc) | ||
|
||
host_name || s3_credentials[:s3_host_name] || "s3.amazonaws.com" | ||
host_name || s3_credentials[:s3_host_name] || "s3.amazonaws.com".freeze | ||
end | ||
|
||
def s3_host_alias | ||
|
@@ -286,7 +286,7 @@ def set_storage_class(storage_class) | |
end | ||
|
||
def parse_credentials creds | ||
creds = creds.respond_to?('call') ? creds.call(self) : creds | ||
creds = creds.respond_to?(:call) ? creds.call(self) : creds | ||
creds = find_credentials(creds).stringify_keys | ||
(creds[RailsEnvironment.get] || creds).symbolize_keys | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,15 +24,16 @@ | |
end | ||
|
||
it "returns the class of the instance" do | ||
class Thing ; end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Space found before semicolon. |
||
attachment = mock | ||
attachment.expects(:instance).returns(attachment) | ||
attachment.expects(:class).returns("Thing") | ||
attachment.expects(:class).returns(Thing) | ||
assert_equal "things", Paperclip::Interpolations.class(attachment, :style) | ||
end | ||
|
||
it "returns the basename of the file" do | ||
attachment = mock | ||
attachment.expects(:original_filename).returns("one.jpg").times(2) | ||
attachment.expects(:original_filename).returns("one.jpg").times(1) | ||
assert_equal "one", Paperclip::Interpolations.basename(attachment, :style) | ||
end | ||
|
||
|
@@ -187,14 +188,14 @@ def url(*args) | |
it "returns the filename as basename.extension" do | ||
attachment = mock | ||
attachment.expects(:styles).returns({}) | ||
attachment.expects(:original_filename).returns("one.jpg").times(3) | ||
attachment.expects(:original_filename).returns("one.jpg").times(2) | ||
assert_equal "one.jpg", Paperclip::Interpolations.filename(attachment, :style) | ||
end | ||
|
||
it "returns the filename as basename.extension when format supplied" do | ||
attachment = mock | ||
attachment.expects(:styles).returns({style: {format: :png}}) | ||
attachment.expects(:original_filename).returns("one.jpg").times(2) | ||
attachment.expects(:original_filename).returns("one.jpg").times(1) | ||
assert_equal "one.png", Paperclip::Interpolations.filename(attachment, :style) | ||
end | ||
|
||
|
@@ -249,4 +250,13 @@ def url(*args) | |
value = Paperclip::Interpolations.interpolate(":notreal/:id/:attachment", :attachment, :style) | ||
assert_equal ":notreal/1234/attachments", value | ||
end | ||
|
||
it "handles question marks" do | ||
Paperclip.interpolates :foo? do | ||
"bar" | ||
end | ||
Paperclip::Interpolations.expects(:fool).never | ||
value = Paperclip::Interpolations.interpolate(":fo/:foo?") | ||
assert_equal ":fo/bar", value | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,34 +3,35 @@ | |
describe 'Plural cache' do | ||
it 'caches pluralizations' do | ||
cache = Paperclip::Interpolations::PluralCache.new | ||
word = "box" | ||
symbol = :box | ||
|
||
word.expects(:pluralize).returns("boxes").once | ||
|
||
cache.pluralize(word) | ||
cache.pluralize(word) | ||
first = cache.pluralize_symbol(symbol) | ||
second = cache.pluralize_symbol(symbol) | ||
expect(first).to equal(second) | ||
end | ||
|
||
it 'caches pluralizations and underscores' do | ||
class BigBox ; end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Space found before semicolon. |
||
cache = Paperclip::Interpolations::PluralCache.new | ||
word = "BigBox" | ||
|
||
word.expects(:pluralize).returns(word).once | ||
word.expects(:underscore).returns(word).once | ||
klass = BigBox | ||
|
||
cache.underscore_and_pluralize(word) | ||
cache.underscore_and_pluralize(word) | ||
first = cache.underscore_and_pluralize_class(klass) | ||
second = cache.underscore_and_pluralize_class(klass) | ||
expect(first).to equal(second) | ||
end | ||
|
||
it 'pluralizes words' do | ||
cache = Paperclip::Interpolations::PluralCache.new | ||
word = "box" | ||
assert_equal "boxes", cache.pluralize(word) | ||
symbol = :box | ||
|
||
expect(cache.pluralize_symbol(symbol)).to eq("boxes") | ||
end | ||
|
||
it 'pluralizes and underscore words' do | ||
it 'pluralizes and underscore class names' do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping. |
||
class BigBox ; end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Space found before semicolon. |
||
cache = Paperclip::Interpolations::PluralCache.new | ||
word = "BigBox" | ||
assert_equal "big_boxes", cache.underscore_and_pluralize(word) | ||
klass = BigBox | ||
|
||
expect(cache.underscore_and_pluralize_class(klass)).to eq("big_boxes") | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
require 'spec_helper' | ||
require 'fog' | ||
require 'fog/aws' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping. |
||
require 'fog/local' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping. |
||
require 'timecop' | ||
|
||
describe Paperclip::Storage::Fog do | ||
|
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.
Redundant
self
detected.