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 4 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!

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