cinchrb / cinch

The IRC Bot Building Framework
http://www.rubydoc.info/gems/cinch
MIT License
1k stars 180 forks source link

Adding new channel_logger plugin as a plugin example. #118

Closed dacamp closed 11 years ago

dacamp commented 11 years ago

I'd like to actually fix the core of the issue in the Logger(s) classes, but right now I don't have time. This will have to do as an example on opening a log for each channel the bot is in. It also creates one log for all private messages. By design connection logging still goes to Logger.

Thanks!

dominikh commented 11 years ago

This could already be implemented with a custom logger, so I'm not sure what you'd want to fix in the core Logger class.

dacamp commented 11 years ago

Yeah overwriting the attribute was dumb and entirely over complicated. I'm going to blame whiskey brain, but in reality I just wasn't thinking... Actually now that I look back at your Logger class I see that it's totally possible to achieve my goal with a custom formatted logger (or not) and your current classes... So, suffice to say this entire plugin is pretty much garbage, I'll close this request. I'll go repent and attempt to cleanse myself of sin. I do think a good example of channel logging is important, I'll try and work on it sometime this week.

It would be nice though to be able to name a Logger though... How opposed would you be to LoggerList becoming a hash rather than an array? It seems like with your current methods, backwards compatibility could still be accomplished. I'm obviously not as familiar with your gem though.

dominikh commented 11 years ago

If LoggerList becomes a hash, the behaviour of iteration will change, << wouldn't work anymore unless we make up a name, etc. So that seems rather backwards incompatible.

What would you need the name for?

dacamp commented 11 years ago

I suppose it's different schools of thought is all. I started looking into this because I thought "What would you need duplicated logging for?" among many other things.

Imagine yourself trying to dig through logs for nearly any piece of information... You would end up in the debug log every time, at which point any other logs are irrelevant. This isn't very DRY:

loggers.first.level = :debug # Now you're getting all levels (:debug) sent to STDERR, cause what would you ever need that changed for?
loggers[1].level    = :log   # Duplicates everything above except for debug
loggers[2].level    = :info  # Duplicates everything above except for debug and log
#  And so on

You're also limiting the total number of logs you'd ever need to open (which should still be considered, but only for heavy use cases will it matter). Since logs can only print out the information set in each level, you'd never create more than LevelOrder.size I/O streams, any further logging is just duplicated information (Albeit maybe formatted differently. Yay colors!). Before you reply with anymore snarky remarks, I realize formatting does change information.

There is so much more to logging than just 'mmm is my code broken?!' Being able to audit IRC logs in the real world is completely different. It forces accountability, so when ~dipshitdev@yolo says "hey I'm gunna push the big red button" you can easily track it down when, where, and hopefully why it was done. During a production outage, post-mordem logs are invaluable.

You wouldn't really lose any of the 'features' your current class has except for "<<" of course. If that is absolutely necessary, it could be written as its own method. As far as iterations go, adding "|k," isn't really so hard is it? Come on man, you gotta see the glass as half full sometimes!

I do think you would gain enough to make it worth it.

# (see Logger#log)                                                          
    def log(messages, event = :debug, level = event)
      self[event].log(messages,event,level)
    end

Auto assign your LoggerLevel to an I/O variable if that level isn't assigned it's own yet. The level of the individual logger would determine if it should actually get logged.

 1.9.3p392 :001 > f = Hash.new($stdout)
  => {} 
 1.9.3p392 :002 > f[:log].puts "I'm a little log bot"
 I'm a little log bot
* NOTE: This could create bloat, if someone decided to pass 'event' param symbols willy nilly... but then again it wouldn't really be cinch's fault since the first time the symbol was instantiated would have been by a user.

There's plenty more, but I'm sure I've worn out my welcome already.

So, rant aside, that's the beauty of open source I suppose. I'll just continue to develop on my own fork.

dominikh commented 11 years ago

The reason we can't lose << is because I do not want to release Cinch 3.0, and breaking backwards compatibility in a minor release is out of the question. So unless you make << work, it's not an option. Same thing goes for iteration, even though that part could actually be made backwards compatible by redefining #each. But then you also got to do that for all the methods of Array, including [], and all the methods of Enumerable that currently return arrays.

This really isn't about me thinking the current design is superior, it's about not breaking backwards compatibility.

There is one thing where I do think the current design is superior, however, and that is being able to the same level to multiple files. Logging isn't just about adding/not adding colours.

I would never accept a mix of FormattedLogger and ZcbotLogger log files to debug a problem.

As for plugins having their own level: This could easily be made possible with the current design, we'd just need to make Logger.LevelOrder configurable. By using Logger#log you can then specify whatever level you wish.

I agree with the per-channel logging, that was an oversight in the design. It's still possible to implement this with the current API, it's just not neat because you would need to parse raw messages in your logger.

As a final, snarky response: The glass is neither half full nor half empty, it's twice as big as it needs to be.

I'll gladly consider this should it ever come to a Cinch 3.0, but until then you might be required to maintain your own fork indeed. Or implement it as a plugin that builds on top of the current infrastructure.