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
---------- Forwarded message ----------
Date: Tue, 24 Jul 2007 11:26:46 +0100 (WEST)
From: Hugh Sasse <hgs@dmu.ac.uk>
Reply-To: rubyonrails-core@googlegroups.com
To: rubyonrails-core@googlegroups.com
Subject: [Rails-core] logging patch
I''ve been using fixture_references, and have been getting errors from
my database about incorrect values inserted. fixture_references turns
off logging by using the silence method. The present implementation gives
me no way to turn that back on temporarily without commenting out code.
I''d like to propose the patch below, or something like it.
I have tried to keep it backwards compatible. I''ve seen no other unit
tests for logging features, so I couldn''t see where to put test
logfiles.
As a consequence this has no tests, but I have used it live and it seems
to be working correctly. I''ve also written it in such a way that
boolean parameters may be avoided. Maybe it can be hammered into a
better shape before acceptance?
Thank you,
Hugh
--- activerecord-1.15.3/lib/active_record/base.rb.orig 2007-04-04
12:10:45.046875000 +0100
+++ activerecord-1.15.3/lib/active_record/base.rb 2007-07-20 11:47:22.234375000
+0100
@@ -861,9 +861,18 @@
end
end
- # Silences the logger for the duration of the block.
- def silence
- old_logger_level, logger.level = logger.level, Logger::ERROR if logger
+ # Silences the logger for the duration of the block. Accepts
+ # a parameter: true :silence, or :silent keeps it silent,
+ # anything else leaves things as they are. The default is
+ # silent, for backwards compatibility, and because it''s what
+ # you normally want.
+ # (This means one need not use obsucure boolean parameters.)
+ def silence(use_silence = true)
+ old_logger_level = logger.level if logger
+ case use_silence
+ when true, :silent, :silence
+ logger.level = Logger::ERROR if logger
+ end
yield
ensure
logger.level = old_logger_level if logger
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---
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 On R, 2007-08-03 at 17:17 +0100, Hugh Sasse wrote:> 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 > > ---------- Forwarded message ---------- > Date: Tue, 24 Jul 2007 11:26:46 +0100 (WEST) > From: Hugh Sasse <hgs@dmu.ac.uk> > Reply-To: rubyonrails-core@googlegroups.com > To: rubyonrails-core@googlegroups.com > Subject: [Rails-core] logging patch > > > I''ve been using fixture_references, and have been getting errors from > my database about incorrect values inserted. fixture_references turns > off logging by using the silence method. The present implementation gives > me no way to turn that back on temporarily without commenting out code. > I''d like to propose the patch below, or something like it.hi > > I have tried to keep it backwards compatible. I''ve seen no other unit > tests for logging features, so I couldn''t see where to put test logfiles. > As a consequence this has no tests, but I have used it live and it seems > to be working correctly. I''ve also written it in such a way that > boolean parameters may be avoided. Maybe it can be hammered into a > better shape before acceptance? > > Thank you, > Hugh > > --- activerecord-1.15.3/lib/active_record/base.rb.orig 2007-04-04 12:10:45.046875000 +0100 > +++ activerecord-1.15.3/lib/active_record/base.rb 2007-07-20 11:47:22.234375000 +0100 > @@ -861,9 +861,18 @@ > end > end > > - # Silences the logger for the duration of the block. > - def silence > - old_logger_level, logger.level = logger.level, Logger::ERROR if logger > + # Silences the logger for the duration of the block. Accepts > + # a parameter: true :silence, or :silent keeps it silent, > + # anything else leaves things as they are. The default is > + # silent, for backwards compatibility, and because it''s what > + # you normally want. > + # (This means one need not use obsucure boolean parameters.) > + def silence(use_silence = true) > + old_logger_level = logger.level if logger > + case use_silence > + when true, :silent, :silence > + logger.level = Logger::ERROR if logger > + end > yield > ensure > logger.level = old_logger_level if logger > > > > > > >--~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
On Fri, 3 Aug 2007, Tarmo Tnav wrote:> > 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 blockYes, 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 blocksWell, 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 --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
On R, 2007-08-03 at 18:51 +0100, Hugh Sasse wrote:> > 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.htmlI 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 --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
On Fri, 3 Aug 2007, Tarmo Tnav wrote:> > On R, 2007-08-03 at 18:51 +0100, Hugh Sasse wrote: > > > 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, &blockWouldn''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 --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
On E, 2007-08-06 at 09:55 +0100, Hugh Sasse wrote:> On Fri, 3 Aug 2007, Tarmo Tnav wrote: > > 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. 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 --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
On Mon, 6 Aug 2007, Tarmo Tnav wrote:> > On E, 2007-08-06 at 09:55 +0100, Hugh Sasse wrote: > > On Fri, 3 Aug 2007, Tarmo Tnav wrote: > > > 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 --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---