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

[Thomas De Schampheleire]

Hi Mads,

Seems I never replied to this feedback...

On Tue, May 12, 2015 at 2:46 AM, Mads Kiilerich <mads at kiilerich.com> wrote:
> 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"?

OK

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

Ok, will fix.

>
>> +        '''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?

I'll write:
Show a message to the user and log it at an appropriate level.

>
>> +        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.

First note that the next commit reduces the log level of 'user errors'
to 'info'.

Then, my reasoning for this mapping is described here:
http://lists.sfconservancy.org/pipermail/kallithea-general/2015q1/000513.html
This is indeed something that I can add in the commit message.

>
> (Nice docstring!)

Thanks :)

>
>> +
>> +        '''
>> +        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.)

What you mean is this, right?

        if log_category is None:
            log_category = {
                'notice': 'info',
                'warning': 'info',
                'error': 'error',
                'success': 'debug',
            }[category]

        {
            'debug': log.debug,
            'info': log.info,
            'warning': log.warning,
            'error': log.error,
        }[log_category]('Flash: %s' % message)

Do you really find this more readable than with the intermediate variables?
For example in the second case, I find it much less obvious that it's
a function call.

Regarding placing these variables outside the function: what is the
reasoning here? Since they are only used and relevant for that
function, isn't it more logical to scope it as such?


Best regards,
Thomas