logging patch (fwd)

I'm reposting this because I got no response. I posted it here
rather than any tracking system because I regard it somewhat as
having wet paint until it's been looked at by those more in the
know than me.
        Hugh

Hi,

Have you created a ticket for this?

I don't see a problem with having this functionality, but your
proposed interface seems a bit strange.

Why do you really need the :silent, and :silence arguments?
They seem to create redundancy: silence(:silence), and don't
really provide additional useful information.

I'd leave the argument boolean only. "silence(false)" and
"silence(true)" or just "silence" all seem obvious in what
they do, that is enable or disable silence.

I'm not sure if there is a need for it, but silence() could actually
support nesting, so you could enable it inside another block
that disables it. All this would require is adding a "when false"
case into this silence implementation which enables some higher
logging level.

Actually, to take this further, the silence method could take
a logging level as a parameter instead of a boolean. This way
you could arbitrarily change the logging level for code blocks
and also have nesting support. Although in that case the name
of the method should probably be changed as well.

just my 2 EURO

Hi,

Have you created a ticket for this?

No, hence my remark about wet paint. I've not sent any
patches to rails yet, so I'm not ready to plunge in the
deep end yet either...

I don't see a problem with having this functionality, but your
proposed interface seems a bit strange.

Why do you really need the :silent, and :silence arguments?

Trying to avoid some mysterious boolean parameter:

http://www.codinghorror.com/blog/archives/000430.html

They seem to create redundancy: silence(:silence), and don't
really provide additional useful information.

On or off would be weird for something that turns output off:
are you turning the offness on, or the onness on? This seemed
the least ambiguous thing I could come up with. But this is
another reason I didn't send it as a straight patch, there's
probably a better, clearer interface to this simple functionality
than is dreamt of in my philosophy....

I'd leave the argument boolean only. "silence(false)" and
"silence(true)" or just "silence" all seem obvious in what
they do, that is enable or disable silence.

But which way round??? Yes, one can look up the default, but
frankly I'd rather not.

I'm not sure if there is a need for it, but silence() could actually
support nesting, so you could enable it inside another block

Yes, I'd not really considered that much, but finer grained control
would be a good thing.

that disables it. All this would require is adding a "when false"
case into this silence implementation which enables some higher
logging level.

Actually, to take this further, the silence method could take
a logging level as a parameter instead of a boolean. This way
you could arbitrarily change the logging level for code blocks

Well, at the risk of getting pecked to death by the Duck Typists
we could check the type of it.

and also have nesting support. Although in that case the name
of the method should probably be changed as well.

I'm really trying to leave this so existing code can be just
changed slightly when I want logging on, despite its author
turning it off. Changing the name would be a bigger change in
code I want to have utilize this. I'd really like to leave
the name the same and have it backwards compatible if I can.

just my 2 EURO

        Thank you,
        Hugh

> I don't see a problem with having this functionality, but your
> proposed interface seems a bit strange.
>
> Why do you really need the :silent, and :silence arguments?

Trying to avoid some mysterious boolean parameter:

http://www.codinghorror.com/blog/archives/000430.html

I suspected this, but including 'true' as a valid option in this
case seemed to go against the intent.

> They seem to create redundancy: silence(:silence), and don't
> really provide additional useful information.

On or off would be weird for something that turns output off:
are you turning the offness on, or the onness on? This seemed
the least ambiguous thing I could come up with. But this is
another reason I didn't send it as a straight patch, there's
probably a better, clearer interface to this simple functionality
than is dreamt of in my philosophy....

Fair enough, the real problem is that the method is called
'silence', I don't think it's common to add arguments to
methods that completely disable any effect the method adds
which is why this will seem unclear whatever way you do it.

The issue you had could technically have been fixed also
by adding a blank method that just calls it's block and
then you could have replaced a call to 'silence' with that method.
This method however is certainly not something that Rails should
provide.

Yes, I'd not really considered that much, but finer grained control
would be a good thing.

> that disables it. All this would require is adding a "when false"
> case into this silence implementation which enables some higher
> logging level.
>
> Actually, to take this further, the silence method could take
> a logging level as a parameter instead of a boolean. This way
> you could arbitrarily change the logging level for code blocks

Well, at the risk of getting pecked to death by the Duck Typists
we could check the type of it.

> and also have nesting support. Although in that case the name
> of the method should probably be changed as well.

I'm really trying to leave this so existing code can be just
changed slightly when I want logging on, despite its author
turning it off. Changing the name would be a bigger change in
code I want to have utilize this. I'd really like to leave
the name the same and have it backwards compatible if I can.

Actually I was thinking more along the lines of adding a more generic
method and then implementing silence as a call to that method.

Something like this:

def use_log_level(new_logger_level)
  old_logger_level, logger.level = logger.level, new_logger_level if
logger
  yield
ensure
  logger.level = old_logger_level if logger
end

def silence(&block)
  use_log_level Logger:ERROR, &block
end

This would allow arbitrarily nesting the calls:
(Imagine that this is not actually one method,
but the calls to use_log_level and silence happen
in different methods which call eachother)

silence do
  unimportant stuff

  use_log_level(Logger::WARN) do
    half-important stuff
  end

  unimportant stuff

  use_log_level(Logger::DEBUG) do
    currently developing this
  end

  unimportant stuff
end

> > I don't see a problem with having this functionality, but your
> > proposed interface seems a bit strange.
> >
> > Why do you really need the :silent, and :silence arguments?
>
> Trying to avoid some mysterious boolean parameter:
>
> http://www.codinghorror.com/blog/archives/000430.html

I suspected this, but including 'true' as a valid option in this
case seemed to go against the intent.

Trying to meet both expectations, really. I was attempting to get it
to behave as people would expect, so they wouldn't need to look at the
source.

> > They seem to create redundancy: silence(:silence), and don't
> > really provide additional useful information.
>
> On or off would be weird for something that turns output off:
> are you turning the offness on, or the onness on? This seemed
> the least ambiguous thing I could come up with. But this is
> another reason I didn't send it as a straight patch, there's
> probably a better, clearer interface to this simple functionality
> than is dreamt of in my philosophy....

Fair enough, the real problem is that the method is called
'silence', I don't think it's common to add arguments to
methods that completely disable any effect the method adds
which is why this will seem unclear whatever way you do it.

The issue you had could technically have been fixed also
by adding a blank method that just calls it's block and
then you could have replaced a call to 'silence' with that method.
This method however is certainly not something that Rails should
provide.

> Yes, I'd not really considered that much, but finer grained control
> would be a good thing.
>
> > that disables it. All this would require is adding a "when false"
> > case into this silence implementation which enables some higher
> > logging level.
> >
> > Actually, to take this further, the silence method could take
> > a logging level as a parameter instead of a boolean. This way
> > you could arbitrarily change the logging level for code blocks
>
> Well, at the risk of getting pecked to death by the Duck Typists
> we could check the type of it.
>
> > and also have nesting support. Although in that case the name
> > of the method should probably be changed as well.
>
> I'm really trying to leave this so existing code can be just
> changed slightly when I want logging on, despite its author
> turning it off. Changing the name would be a bigger change in
> code I want to have utilize this. I'd really like to leave
> the name the same and have it backwards compatible if I can.

Actually I was thinking more along the lines of adding a more generic
method and then implementing silence as a call to that method.

Something like this:

def use_log_level(new_logger_level)
  old_logger_level, logger.level = logger.level, new_logger_level if
logger
  yield
ensure
  logger.level = old_logger_level if logger
end

def silence(&block)
  use_log_level Logger:ERROR, &block

Wouldn't that need to be higher than ERROR, i.e. a new value, possibly
called NEVER or something?

end

This would allow arbitrarily nesting the calls:
(Imagine that this is not actually one method,
but the calls to use_log_level and silence happen
in different methods which call eachother)

silence do
  unimportant stuff

  use_log_level(Logger::WARN) do
    half-important stuff
  end

  unimportant stuff

  use_log_level(Logger::DEBUG) do
    currently developing this
  end

  unimportant stuff
end

        Hugh

Not neccessarily, the silence method currently used by
Rails uses the ERROR log level.

I can't really see a good reason to make the silence
method hide error messages, though perhaps it would
be good to provide some other way to do so. Like
use_log_level(Logger::NONE) or something like that.

  Tarmo

> > def silence(&block)
> > use_log_level Logger:ERROR, &block
>
> Wouldn't that need to be higher than ERROR, i.e. a new value, possibly
> called NEVER or something?

Not neccessarily, the silence method currently used by
Rails uses the ERROR log level.

Oh, OK, I was just thinking of total silence....

I can't really see a good reason to make the silence
method hide error messages, though perhaps it would
be good to provide some other way to do so. Like
use_log_level(Logger::NONE) or something like that.

agreed.

  Tarmo

        Hugh