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:
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