Pass DGRAM socket as a parameter, not a block #2
Conversation
| super(SysLogger::IO.new(&block), shift_age, shift_size) | ||
| def initialize(logdev = nil, shift_age = 0, shift_size = 1048576) | ||
| if logdev.is_a?(Proc) | ||
| super(SysLogger::IO.new(logdev), shift_age, shift_size) |
There was a problem hiding this comment.
Does this actually work? I think it needs to be Syslogger::IO.new(&logdev). Anyways, I would rather just keep the syntax we have. Seems more standard to just accept a block that creates the logdev.
There was a problem hiding this comment.
SysLogger::IO expects it to be a proc because it does .call by default.
& means that you passing proc as a block (last argument)
| module Creators | ||
| def self.unix_dgram_socket(socket_path) | ||
| proc { | ||
| Proc.new { |
There was a problem hiding this comment.
This is a no-op in the versions of ruby that this library supports (2+).
There was a problem hiding this comment.
I don't really care. The source I looked at (http://batsov.com/articles/2014/02/04/the-elements-of-style-in-ruby-number-12-proc-vs-proc-dot-new/) says to prefer proc over Proc.new, but I only googled it for 3 seconds and don't know if that's the prevailing Ruby style elsewhere.
There was a problem hiding this comment.
Well, look at this like raise which does Exception.new. You don't like it, right?
There was a problem hiding this comment.
Ha, he is talking about ruby 1.8 and 1.9... they are dead
There was a problem hiding this comment.
I'm fine with raise FooException.new(arg1, arg2) just like I'm fine with proc { body }. But, like I said, I don't really care, I was just going off one style guide I looked at, since they're exactly identical constructs. I think the bigger issue is that I agree with @att14 that it's clearer to just pass a block using block-syntax rather than treating the first argument differently depending on its type, and I don't think the proc versus Proc.new difference is worth changing if it ends up being the only thing in this PR.
There was a problem hiding this comment.
Ruby style suggests duck typing over different combinations of arguments.
You are using MonoLogger, it means that I should be able to replace Syslogger with MonoLogger and don't have to change other code.
There was a problem hiding this comment.
Don't get me wrong -- I think the entire logger interface in Ruby is braindead and awful. I would much prefer that we drop the arguments to initialize entirely and switch to something sane. It's a balancing act between having an interface which is vaguely similar to the existing (again, imo absolutely awful) interface and doing the right thing.
|
👎 ? |
|
This still needed? |
Current initializer limit the usage of Syslogger. If I want to prepare
logdevoutside of method whereSyslogger.newcalled I have to use this::SysLogger::IO.new(&::SysLogger::Creators::unix_dgram_socket(ENV["LOG_DGRAM_SYSLOG"]))