452686 - [silme] Implement a basic error class for Silme

Status: RESOLVED FIXED

(Mozilla Localizations :: Infrastructure, defect)

Description

[Adrian Kalla [:adriank]]

Attachment: proposed pacth which implements an Error class

We need better error handling in Silme than now: always raising an exception is not a good idea.

The patch is based on conclusions of a discussion I had with Gandalf and Axel last Friday.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 335963 [details] [diff] [review]
proposed pacth which implements an Error class

>diff --git a/lib/mozilla/l10n/error.py b/lib/mozilla/l10n/error.py
>new file mode 100644
>--- /dev/null
>+++ b/lib/mozilla/l10n/error.py
>@@ -0,0 +1,27 @@
>+class Error:
>+    def __init__(self):
>+        self.errors = []
>+
>+    def addError(self, type, error):
>+        if isinstance(type,str):
>+            self.errors.append((type, error))
>+        else:
>+            raise Exception("The first argument must be of type 'str'.")

shorter

def addError(self, type, error):
  if not isinstance(type,str);
    raise Exception()
  self.errors.append((type, error))


>+    def getErrors(self, type='all'):
>+        if type is 'all':
>+            return self.errors
>+        else:
>+            allerrors = self.errors
>+            wantederrors = []
>+            while len(allerrors) > 0:
>+                error = allerrors.pop(0)
>+                if isinstance(type,tuple):
>+                    if error[0] in type:
>+                        wantederrors.append(error)
>+                else:
>+                    if error[0] is type:
>+                        wantederrors.append(error)
>+            return wantederrors

def getErrors(self, type='all'):
  ...
  else:
    return [item for item in self.errors if item in type]

Beside, please, use 2 spaces for indent.
And consider adding static to make the class static.

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Taking.

We have three options now:

1) Use default logger
2) Use Akalla's Error class
3) Use Stef's logger class

I'm investigating all options

Comment 3

[Axel Hecht [:Pike]]

What I care about is being able to switch between silme and tooling for compare-locales with minimal cost and risk.

I'm not sure how that should work, but in the end, I'd like errors to show up in the same json file as missing and obsolete strings or files.

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

I'm experimenting with logging modile and added silme.core.logger class: http://hg.mozilla.org/users/zbraniecki_mozilla.com/silme/file/c0fe398f409b/lib/silme/core/logger.py

I'm trying to make sure that I can cover everything Adrian and Stef need from logging and make it as flexible as possible (making it flexible is easy with logging).
Last thing is testing performance cost here.

Comment 5

[Axel Hecht [:Pike]]

This is an example output I have right now, http://l10n.mozilla.org/buildbot/compare/hg-comm-langpack/1256. I don't see how to get to that data straight out of the box from a logger.

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

Adrian, stef:

what do you think about adding simple

self.log = [] to each class and then addLog() and getLogs() ?

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: external logger proposal

After careful consideration I decided to give up on the idea to use standard logging module here. It was not suited for our needs.
I still believe that it would be better to use it because of how nicely it could mix with other logging features and other libs that use logging, but I'm giving up for now.

Instead I'm proposing a trivial logging module which can be implemented in one of two ways.

1) Inline. By adding functions to each class.
2) Externally. By creating a new, optional module silme.core.logger or silme.logger that will cover this.

I'm attaching an option with an external logger, since it simplifies the code and gives us ability not to use it when not needed.

In this, early, approach, logging does not happen inside silme lib, but can be done by apps. I'm playing with an idea of making logger requirement and using it inside the lib.

Good thing about it is that API is so simple and generic that even if we'll switch to logging one day we will be able to use.

Now. I'm not sure if we want to extend it to make it possible to log file/line where the log entry has been added or not, I'm also not sure if its better to use "logs" or "entries" as a variable/param name.

What I know is that it works and is simple. It should cover Adrian's needs and Stef's needs and I hope Pike will like it :)

Comments and feedback more than welcomed!

Comment 8

[Stefan Plewako [:stef]]

I really like external logger idea, with some standard for adding messages it could save me a lot of work.

In this way I would need to override only one core file (since it seems that one, good for everybody, solution simply don't exist) and by default Silme could use anything (eg standard logger).

This would probably require standard (possibly described in module) for "message" (type, values, etc), standard method names, keeping all other messages structure away from main api and would be probably most flexible solution.

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

I'm marking this as fixed since I checked in an initial logger.

I may extend its API before freeze, but that's it. You can easily overload it with any other logger that you need.