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

[Thomas De Schampheleire]

On Sun, Mar 29, 2015 at 4:50 AM, Mads Kiilerich <mads at 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
>> <patrickdepinguin at gmail.com> 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 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