asenchi / scrolls

Simple logging
MIT License
158 stars 26 forks source link

Definition of Scrolls#log_exception is confusing. #72

Closed sylvain-8422 closed 2 years ago

sylvain-8422 commented 3 years ago

Order of Scrolls#log_exception parameters has been reversed in 0.9.0:

https://github.com/asenchi/scrolls/blob/6973a356e3f649bbc96101f21cb6b0199ea5de87/test/test_scrolls.rb#L129-L131

https://github.com/asenchi/scrolls/blob/6973a356e3f649bbc96101f21cb6b0199ea5de87/lib/scrolls/logger.rb#L160

But definition and documentation block of Scrolls#log_exception are still using log_exception(data, e):

https://github.com/asenchi/scrolls/blob/6973a356e3f649bbc96101f21cb6b0199ea5de87/lib/scrolls.rb#L72-L92

asenchi commented 3 years ago

This is fixed in https://github.com/asenchi/scrolls/pull/74.

sylvain-8422 commented 3 years ago

The most confusing part is the method definition, and it is still in reverse order:

https://github.com/asenchi/scrolls/blob/f4c5be7a2edbfaa02ff4d1e38a1afa4f61b2b03a/lib/scrolls.rb#L87-L92

It should be:

  def log_exception(e, data)
    # Allows us to call #log directly and initialize defaults
    @log = Logger.new({}) unless @initialized

    @log.log_exception(e, data)
  end
asenchi commented 3 years ago

Wow, lol! I looked at the test and saw the right order and didn't even check the function. I'll get this fixed up and perhaps even do some validation here to make sure of the order.

Sorry about that, I looked quickly this morning and wanted to get a response/fix out.

asenchi commented 2 years ago

This issue was fixed in #75