-
-
Notifications
You must be signed in to change notification settings - Fork 259
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
Rails/EagerEvaluationLogMessage
should always report an offense when not passed a block
#1355
Comments
There is really no reason to restrict passing a plain literal. With frozen string literals this is not a problem and passing a block is in fact slower: # frozen_string_literal: true
require "bundler/inline"
gemfile(true) do
source "https://rubygems.org"
gem "rails"
gem "benchmark-ips"
end
require "minitest/autorun"
require "active_model"
def bar
"bar"
end
Rails.logger = Logger.new(STDOUT)
Rails.logger.level = :info
Benchmark.ips do |x|
x.report("literal") do
Rails.logger.debug "some_string"
end
x.report("literal_block") do
Rails.logger.debug { "some_string" }
end
x.report("interpolated") do
Rails.logger.debug "some_string#{bar}"
end
x.report("interpolated_block") do
Rails.logger.debug { "some_string#{bar}" }
end
x.report("method_call") do
Rails.logger.debug bar
end
x.report("method_call_block") do
Rails.logger.debug { bar }
end
end
For the method, I guess it could register an offense there. I find it highly unlikely that it would just return a frozen string and as such will likely be more performant. |
@Earlopain I have tried to benchmark the behaviour of # frozen_string_literal: true
require "bundler/inline"
gemfile(true) do
source "https://rubygems.org"
gem "rails"
gem "benchmark-ips"
end
require "minitest/autorun"
require "active_model"
var = "var"
Rails.logger = Logger.new(STDOUT)
Rails.logger.level = :info
Benchmark.ips do |x|
x.report("variabile") do
Rails.logger.debug var
end
x.report("variable_block") do
Rails.logger.debug { var }
end
end
It seems that when dealing with variables it's also faster to pass the positional argument instead of a block 🤔. So I guess this is becoming a little bit too much to handle? I don't think Rubocop can discern wether the argument passed to |
It can distinguish between local variables and other things |
Is your feature request related to a problem? Please describe.
Currently, the
Rails/EagerEvaluationLogMessage
cop, when enabled manually, will only report offenses whenRails.logger.debug
receives as argument an interpolated string directly.Look at this, for example:
Now, this cop was created because when the
Rails.logger.debug
method receives a string as first argument, Ruby will always allocate that string in memory even if the log level for the current Rails environment is:info
or above (and thusRails.logger.debug
doesn't actually debug anything).In the example above, case B is correctly flagged since it results in an unneeded allocation of an interpolated string. Case A and C however are still allocation unnecessary objects (assuming that the Rails
log_level
is:info
or above, of course) but are not flagged by the cop.Describe the solution you'd like
I think that the
Rails/EagerEvaluationLogMessage
cop should always report an offense whenRails.logger.debug
isn't passed a block.The text was updated successfully, but these errors were encountered: