[PATCH 2 of 6 v2] helpers: add automatic logging to Flash

[Mads Kiilerich]

On 05/10/2015 08:22 PM, Thomas De Schampheleire wrote:
> # HG changeset patch
> # User Thomas De Schampheleire <thomas.de.schampheleire at gmail.com>
> # Date 1427573281 -3600
> #      Sat Mar 28 21:08:01 2015 +0100
> # Node ID b7e259611340434bead43af0327e5be53275319b
> # Parent  877fa67247ecfe9cb01a6b70bfaeee265bb65b9e
> helpers: add automatic logging to Flash

"add automatic logging of Flash messages shown to users"?

> Log all messages displayed through a flash to the user.
> The log level equals the flash category if it is defined, or 'info'
> otherwise.
>
> 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
> @@ -406,6 +406,43 @@ class _Message(object):
>   
>   class Flash(_Flash):
>   
> +    def __call__(self, message, category=None, ignore_duplicate=False, log_category=None):

Perhaps just make category default to 'info' and avoid special handling 
of None? Or do we explicitly and for good reasons pass None and need 
handling of that?

> +        '''Show a message to the user

"and make sure it also is logged if debug is enabled" ... or how would 
you describe the intended functionality?

> +        category: notice (default), warning, error, success
> +        log_category: info, warning, error, debug
> +
> +        If log_category is not set, the following mapping applies:
> +
> +        category | log_category
> +        ---------+-------------
> +         None    | info
> +         notice  | info
> +         warning | info
> +         error   | error
> +         success | debug

 From another point of view: What is relevant info to the user is not 
necessarily relevant for the admin to get in the log, unless debug 
logging is enabled. User errors are also not application errors and 
should thus not be logged as errors. Thus, all these flash messages 
should always use debug logging (as hinted in my proposed function 
description above) ... or at least default to it.

BUT I think I support your point of view. Some thoughts on this "issue" 
can perhaps be added to the commit message.

(Nice docstring!)

> +
> +        '''
> +        log_function = {
> +            'debug': log.debug,
> +            'info': log.info,
> +            'warning': log.warning,
> +            'error': log.error,
> +        }
> +
> +        if not log_category:

if log_category is None

> +            transform = {
> +                None: 'info',
> +                'notice': 'info',
> +                'warning': 'info',
> +                'error': 'error',
> +                'success': 'debug',
> +            }
> +            log_category = transform[category]
> +
> +        log_function[log_category]('Flash: %s' % message)

I would skip the naming of the log_function and transform dicts and just 
use them directly. I think that would be more readable. (If not, it 
could be argued that these dicts are consts and should be defined 
outside the function.)

> +        super(Flash, self).__call__(message, category, ignore_duplicate)
> +
>       def pop_messages(self):
>           """Return all accumulated messages and delete them from the session.
>   

/Mads