Suggestion: don't assume Rails logger is present; make logger configurable #38

Closed
nathanl opened this Issue Jan 21, 2014 · 3 comments

Projects

None yet

2 participants

@nathanl

I'm joining a non-Rails project that uses this gem. I just ran the tests and got a backtrace that included this:

...../gems/flag_shih_tzu-0.3.2/lib/flag_shih_tzu.rb:209:in `check_flag_column': 
private method `warn' called for nil:NilClass (NoMethodError)

That goes to a line like this (on master):
https://github.com/pboling/flag_shih_tzu/blob/master/lib/flag_shih_tzu.rb#L269

Turns out that you're trying to give me a helpful error, but I actually got a very confusing one. 😆

This line assumes that the Rails logger is present (right?).

An approach I've seen that gets around that is to have a configurable logger. Eg, when you want to log, call FlagShihTzu.logger.warn('whatever'). That would be a method like:

module FlagShihTzu
  def self.logger
    @logger ||= configuration.logger
  end
end

Then users could configure your log messages to point wherever they want in a configuration file. Eg, configuration.logger = Rails.logger in a Rails app, or configuration.logger = Logger.new(my_location), where my_location could be 'some_file.log' or STDOUT or '/dev/null'.

This is a trick @adamhunter showed me, which we used in Authority: https://github.com/nathanl/authority/blob/c4e55d89389904cda888cbc01a0864728cfde950/lib/authority.rb#L61

Just an idea.

@pboling
Owner

I do exactly that on many of my gems. I just haven't had time to implement it on this one! It's on my mental list of things to get done. Thanks for documenting it as an issue!

@pboling
Owner

I have removed the logger altogether. Now just using Kernel#warn across the board.

@pboling
Owner

I think for now that is sufficient, but pull request would be welcome with a real customizable logger implementation.

@pboling pboling closed this Jul 14, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment