Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Added integration with the built-in logging package #440

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

dylan-robins
Copy link

@dylan-robins dylan-robins commented Nov 3, 2024

Closes: #49

This PR integrates the built-in logging package, as listed in the Cleo 3.0 writeup (#415 ). Implementation is of course subject to change, but I thought I'd get the ball rolling :)

Here's what I did:

  • Added an OutputHandler class, allowing logging to emit LogRecords to a cleo.io.outputs.output.Output
  • Added a method to cleo.application.Application which attaches an OutputHandler to the root logger, and configures it to have a log level that is coherent with Cleo's one.
  • Added unit tests ensuring all this works as expected.

Here are some open points that I know we need to discuss:

  • How should we map logging's log levels to Cleo's verbosity levels? Current implementation is as follows:
    • Quiet: CRITICAL (but nothing gets printed anyway so whatever)
    • Normal: WARNING
    • Verbose: INFO
    • Very verbose: DEBUG
    • Debug: DEBUG
  • Formatting:
    • Should the stuff coming from the logging handler have some sort of prefix or something to distinguish them from what is coming directly from output.write_line()?
    • The formatter styles for error, warning, info and debug need to be defined.

@Secrus
Copy link
Member

Secrus commented Nov 18, 2024

Hey, first of all, thank you for your contribution. A couple of things to start with:

  1. Logging integration shouldn't be automatic. We should only provide a logging.Handler class (CleoHandler maybe?) and leave the rest to the user.
  2. I believe we can have a deeper integration and provide more from Cleo than just wrapping io.write into a logging handler. See what Rich does in their logging module. It won't be 1:1, but it should give you some ideas.
  3. The code should be moved to cleo.logging module.

@dylan-robins
Copy link
Author

Thanks for the feedback, I'll look into everything you said and update my branch accordingly. I'll let you know when I have something worth reviewing!

@dylan-robins
Copy link
Author

dylan-robins commented Jan 4, 2025

  • Logging integration shouldn't be automatic. We should only provide a logging.Handler class (CleoHandler maybe?) and leave the rest to the user.
  • I believe we can have a deeper integration and provide more from Cleo than just wrapping io.write into a logging handler. See what Rich does in their logging module. It won't be 1:1, but it should give you some ideas.
  • The code should be moved to cleo.logging module.

The non-automatic integration is giving me a bit of a headache. The CleoHandler class needs a reference to the application's Output object in order to be able to write to it, however Application.io doesn't exist until the application is running. This means that the user can't initialise the handler before calling app.run()...

Any thoughts on how to proceed?

I have two approaches in mind:

  1. Refactor cleo.Application to make it possible to inject a pre-initialized io object
  2. Keep the _initialize_logger() method I added to Application, but make it opt-in using an argument to __init__

@dylan-robins
Copy link
Author

Erm forget that last comment, I'm dumb, obviously I can just create my Output object and pass it to the Application.run function. Duh 🙃

@dylan-robins
Copy link
Author

  • Logging integration shouldn't be automatic. We should only provide a logging.Handler class (CleoHandler maybe?) and leave the rest to the user.
  • I believe we can have a deeper integration and provide more from Cleo than just wrapping io.write into a logging handler. See what Rich does in their logging module. It won't be 1:1, but it should give you some ideas.
  • The code should be moved to cleo.logging module.

Integration is now fully handled on the user side. I've updated the tests to reflect this, and this is a working example of user code:

import logging
import sys

from cleo.application import Application
from cleo.io.outputs.stream_output import StreamOutput
from cleo.logging.cleo_handler import CleoHandler
from tests.fixtures.foo4_command import Foo4Command


if __name__ == "__main__":

    # Configure a cleo single-command application
    app = Application()
    cmd = Foo4Command()
    app.add(cmd)
    app._default_command = cmd.name

    # Create the custom handler that'll redirect logging's prints to be run through Cleo's output
    output = StreamOutput(sys.stdout)
    handler = CleoHandler(output)
    # The user can set their format as usual, albeit with support for cleo format strings!
    fmt = logging.Formatter(
        "<fg=light_gray;options=reverse>%(asctime)s</> - %(levelname)s: %(message)s",
        datefmt='%m/%d/%Y %I:%M:%S %p',
    )
    handler.setFormatter(fmt)

    # Register the handler
    root = logging.getLogger()
    root.addHandler(handler)
    root.setLevel(logging.NOTSET) # IMPORTANT so that the root logger respects cleo's verbosity levels

    # run the cleo application
    app.run(output=output, error_output=output)

Result:
image

image

I'll start looking into what Rich do for formatting and continue updating this PR

@Secrus
Copy link
Member

Secrus commented Jan 12, 2025

Thanks for working on that @dylan-robins, it looks great so far.

@Secrus Secrus mentioned this pull request Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use cleo in combination with logging
2 participants