Swell logger extends logging.Logger

[mranst]

Description

#428 – This PR implements the swell logger as an extension of the logging library. This means that Swell loggers are now handled in a similar way to the base library.

The Logger class in the logging library is invoked through the getLogger method, rather than directly. This method goes through a manager class, which handles initialized logger objects. This PR introduces the get_logger method, which overrides the base logging.Manager object so it uses the swell Logger. Functions from Logger such as info() override the base functions, so the behavior doesn't change. Invoking Logger directly will still work, but if accepted, logger instances should ideally be invoked using get_logger in the future.

[ashiklom]

Nice work!

Though, now that you've gone through and replaced Logger(...) with get_logger(...) throughout (which I approve!), I think we might as well consider deprecating our custom Logger class altogether and just teaching get_logger to create logging.Logger objects with the semantics we expect.

[ashiklom]

Strictly speaking, if you are doing inheritance, you should obey the Liskov Substitution Principle; i.e., if a swell.Logger object is a subclass of logging.Logger, then you should be able to use a swell.Logger anywhere where you can use logging.Logger object.

In practice, the specific complaint here is that the __init__ methods for logging.Logger and swell.logging.Logger have incompatible argument signatures. Something like this, swell.logging.Logger(name="this", level="DEBUG"), will throw an error, but under a strict inheritance hierarchy, this should work.

A simple fix is to (1) change task_name to just name (for consistency with logging.Logger and (2) accept **kwargs as an optional argument that gets passed on via super().__init__(name, **kwargs).

[mranst]

Nice work!

Though, now that you've gone through and replaced Logger(...) with get_logger(...) throughout (which I approve!), I think we might as well consider deprecating our custom Logger class altogether and just teaching get_logger to create logging.Logger objects with the semantics we expect.

I think this makes a lot of sense, pretty much everything should be able to go through logging internals. The one exception might be the abort function, which seems very convenient and doesn't seem to have an obvious alternative in logging (though I'm still looking into it). At the very least, I think I can accomplish the same thing with a much simpler extension of Logger than I currently have.

[mranst]

I tried to implement some more elegant solutions to this problem, but ended up sort of going in a circle. Logging actually doesn't seem to have very complex formatting for message levels. The implementation now at least makes better use of inheritence for the logging levels.

The customization for detail is simpler now. Setting the LOGLEVEL environment variable will control the minimum priority logging level, and by default it will include messages at INFO or above. This is in line with how logging usually works.

[Dooruk]

Is this still active and pending for a review? Figure it might have been forgotten in other numerous PRs 😃

[mranst]

Is this still active and pending for a review? Figure it might have been forgotten in other numerous PRs 😃

Yeah, I had sort of forgotten about it too. I think I had it where I wanted it on the latest commit

[ashiklom]

Magic number. Can you add a comment to what 60 means here?

[ashiklom]

Pending one tiny fix (add a comment about what loglevel(60) means), and assuming this passes all our tests, we can get this merged.

Eventually, I still would like us to remove our own logging.Logger class in favor of using Python's builtin logger only where appropriate (for logging; not for error handling or taking user input) and using more appropriate tools for managing other things. But for now, let's get this in and tackle the rest of the problem later.