only warn logger if present

[andyweiss1982]

Currently it is possible to run into this error message: private method 'warn' called for nil:NilClass, which will crash apps that do not have ActiveRecord::Base.logger set.

It's a little confusing to draw a straight line from the original source of the error ("Attribute #{k} does not exist on #{version.item_type} (Version id: #{version.id}).") to the error message currently displayed in the console.

I would think it would be preferable to only call warn on a logger if one is present, eliminating possible sources of 500 errors and allowing developers to use paper_trail without explicitly setting a logger class for PaperTrail::Version or ActiveRecord::Base

[batter]

What does the ampersand character do in this context?

Does it not make more sense to just wrap these statements on a if block that checks to see if version.logger is not nil?

[batter]

Ok, I think I understand now, however according to this stackoverflow post, this functionality was introduced in Ruby 2.3, which would make the gem unusable with earlier Ruby versions, which is not desirable.

[andyweiss1982]

OK that is certainly a fair critique. I can resubmit with the conditional inlined explicitly.

[batter]

That would be great. I think we can just wrap it in a if version.logger...

The question is whether there should be an else which logs it to STDOUT or something, do you have thoughts on that?

[andyweiss1982]

@batter what do you make of the most recent change, replacing version.logger with version.logger || Logger.new(STDOUT)?

[jaredbeck]

@batter what do you make of the most recent change, replacing version.logger with version.logger || Logger.new(STDOUT)?

This will prevent people from disabling output, which I assume is the goal of setting ActiveRecord::Base.logger to nil. So, I don't think it's a good idea.

[batter]

I suppose @jaredbeck is probably right. Personally I would prefer to see feedback but I know not everyone wants that, and we've gotten complaints in the past from some users who had difficulty silencing warnings that PaperTrail was passing, so I think the original proposed if statement wrap is probably the best approach.

[andyweiss1982]

@batter sounds good, changed it back to plain vanilla if version.logger

[batter]

Looks good to me, and the tests are passing. Can you squash this into a single commit, then I think it's ready to merge, cheers!

[andyweiss1982]

Great, all squashed. Thanks!

[batter]

Cheers!

[jaredbeck]

Released in 6.0.2