Re: [PATCH 1 of 2] helpers: add automatic logging to Flash

2015-03-29 Thread Thomas De Schampheleire
On Sun, Mar 29, 2015 at 4:50 AM, Mads Kiilerich m...@kiilerich.com wrote:
 On 03/28/2015 10:35 PM, Thomas De Schampheleire wrote:

 On Sat, Mar 28, 2015 at 10:34 PM, Thomas De Schampheleire
 patrickdeping...@gmail.com wrote:

 # HG changeset patch
 # User Thomas De Schampheleire thomas.de.schamphele...@gmail.com
 # Date 1427573281 -3600
 #  Sat Mar 28 21:08:01 2015 +0100
 # Node ID 2f4d67b8acefc3e85ddb5c165bc4054b0ddab90d
 # Parent  65c5e70a1d0c1861d7dc835f204d132342920da2
 helpers: add automatic logging to Flash

 Log all messages displayed through a flash to the user.
 The log level equals the flash category if it is defined and not
 'success',
 otherwise falls back to 'debug'.

 Subsequent patches can change the log level for individual flash calls to
 make the log more useful.

 diff --git a/kallithea/lib/helpers.py b/kallithea/lib/helpers.py
 --- a/kallithea/lib/helpers.py
 +++ b/kallithea/lib/helpers.py
 @@ -423,6 +423,29 @@

   class Flash(_Flash):

 +def __call__(self, message, category=None, ignore_duplicate=False,
 log_category=None):
 +'''Show a message to the user
 +
 +category: notice (default), warning, error, success
 +log_category: debug, notice, warning, error, success
 +
 +If log_category is not set, it defaults to 'debug' unless
 category is
 +set and different from 'success'.
 +'''
 +log_function = {
 +'debug': log.debug,
 +'notice': log.info,
 +'success': log.info,
 +'warning': log.warning,
 +'error': log.error,
 +}
 +
 +if not log_category:
 +log_category = category if category and category !=
 'success' else 'debug'
 +
 +log_function[log_category]('Flash: %s' % message)
 +super(Flash, self).__call__(message, category, ignore_duplicate)
 +
   def pop_messages(self):
   Return all accumulated messages and delete them from the
 session.


 We could consider mapping 'category=warning' on 'log_category=debug'
 too, as most such flashes are recategorized as such in the next patch.
 In most cases, a warning flash means a user mistake/disallowed action
 but not a real problem.


 Hmm ... I guess this boils down to 3 cases:
 1: the user did something wrong - do it better next time and everything is
 fine
 2: help the admin debug the setup or why the user action was wrong
 3: help the developer debug during development or in a bug report

 I don't know ... but it would be nice to have some standards for how we
 handle these situations ... and it seems like developer debug and admin
 debug is something different ...

In case of developer debug, I think we can expect that developer to
manually raise the log-level to DEBUG, no?
For such messages, 'debug' would be fine.

Assuming an admin enables debug when a user reports a problem is too
late: the user typically does not like to try and reproduce an issue,
if it is reproducible at all (or maybe the user did not remember
exactly what he did). So for this case, a log-level of 'info' would be
better, which is shown by default.

Handling both cases at the same time is not possible if they don't
line up, unless we introduce a special switch to differentiate them,
but I think it's overkill.

So what about:

category -- log_category

success -- debug
notice -- info
warning -- info
error -- error

if no category specified, default to 'notice' and thus log.info.



 Also, I don't know if we like or dislike _ or camel/pascal casing as in
 log_category. Leaving it undefined would however cause confusion. We
 should make up our mind and put it in contributins.rts . The good thing
 about the Mercurial convention is that it at least has an opinion ...

Looking at the current Kallithea sources, I see:

- classes (incl. decorators) are named in CamelCase
- methods and arguments are named with lowercase_underscores

I find this a perfectly logical and clear naming strategy.
It's not the same one as Mercurial's coding style, which uses all
lowercase without underscores, even for classes (except
ExceptionClasses).
http://mercurial.selenic.com/wiki/CodingStyle#Naming_conventions

Best regards,
Thomas
___
kallithea-general mailing list
kallithea-general@sfconservancy.org
http://lists.sfconservancy.org/mailman/listinfo/kallithea-general


Re: [PATCH 1 of 2] helpers: add automatic logging to Flash

2015-03-28 Thread Thomas De Schampheleire
On Sat, Mar 28, 2015 at 10:34 PM, Thomas De Schampheleire
patrickdeping...@gmail.com wrote:
 # HG changeset patch
 # User Thomas De Schampheleire thomas.de.schamphele...@gmail.com
 # Date 1427573281 -3600
 #  Sat Mar 28 21:08:01 2015 +0100
 # Node ID 2f4d67b8acefc3e85ddb5c165bc4054b0ddab90d
 # Parent  65c5e70a1d0c1861d7dc835f204d132342920da2
 helpers: add automatic logging to Flash

 Log all messages displayed through a flash to the user.
 The log level equals the flash category if it is defined and not 'success',
 otherwise falls back to 'debug'.

 Subsequent patches can change the log level for individual flash calls to
 make the log more useful.

 diff --git a/kallithea/lib/helpers.py b/kallithea/lib/helpers.py
 --- a/kallithea/lib/helpers.py
 +++ b/kallithea/lib/helpers.py
 @@ -423,6 +423,29 @@

  class Flash(_Flash):

 +def __call__(self, message, category=None, ignore_duplicate=False, 
 log_category=None):
 +'''Show a message to the user
 +
 +category: notice (default), warning, error, success
 +log_category: debug, notice, warning, error, success
 +
 +If log_category is not set, it defaults to 'debug' unless category is
 +set and different from 'success'.
 +'''
 +log_function = {
 +'debug': log.debug,
 +'notice': log.info,
 +'success': log.info,
 +'warning': log.warning,
 +'error': log.error,
 +}
 +
 +if not log_category:
 +log_category = category if category and category != 'success' 
 else 'debug'
 +
 +log_function[log_category]('Flash: %s' % message)
 +super(Flash, self).__call__(message, category, ignore_duplicate)
 +
  def pop_messages(self):
  Return all accumulated messages and delete them from the session.



We could consider mapping 'category=warning' on 'log_category=debug'
too, as most such flashes are recategorized as such in the next patch.
In most cases, a warning flash means a user mistake/disallowed action
but not a real problem.
___
kallithea-general mailing list
kallithea-general@sfconservancy.org
http://lists.sfconservancy.org/mailman/listinfo/kallithea-general