Skip to content
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

Setting Embedly logger to Rails.logger in Rails v4 seems to stop Rails logging from working #22

Open
floehopper opened this issue Jul 1, 2013 · 4 comments

Comments

@floehopper
Copy link

Adding the following in an initializer worked ok in Rails v3.2.13, but seemed to stop all logging output after upgrading to Rails v4.0.

Embedly.configure do |config|
  config.logger = Rails.logger
end
@romanbsd
Copy link
Contributor

👍

The problem is that Embedly changes the log_level of the provided logger. In fact, it shouldn't touch the externally provided logger, but only set the log level if the logger was created by embedly itself. The workaround for time being is the following monkey patch:

class Embedly::Configuration
  def set_logger_level(true_or_false)
  end
end

@dokipen
Copy link
Contributor

dokipen commented Mar 13, 2014

Thanks. Any chance I can get a pull request to fix this for real?

@romanbsd
Copy link
Contributor

Frankly, I don't like the way it's currently designed. So I don't see a good way to fix it properly without breaking the current api. I would say that Embedly shouldn't do the log level setting at all. It should receive a Logger or instantiate one, expose the logger instance, and then the user can change the log level at will. For the most cases, I assume that the users are re-using some logger (probably Rails logger), so they would prefer to set the log level globally and that's it.

And if you really want fine grain control over debugging things, then maybe it's better to have a debug method which would do something like:

def debug(&blk)
  if debug?
    logger.info(&blk)
  end
end

@dokipen
Copy link
Contributor

dokipen commented Mar 13, 2014

I think the only reason that setting the level is in there is for the CLI to support --verbose. Over time it somehow got lumped into "configuration". That should probably be moved to the CLI related code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants