backgroundremover icon indicating copy to clipboard operation
backgroundremover copied to clipboard

Add logging to replace print satement for debugging

Open JustOscarJ1 opened this issue 1 year ago • 2 comments

Replaces the hardcoded print statement used for debugging with a configurable logging setup. Specifically:

Replaces print(f"DEBUG: path to be checked: {path}") with logger.debug(f"path to be checked: {path}"). Introduces a module-level logger that doesn't propagate, to avoid affecting other parts of the library or user's project. Maintains the same output format for consistency. Allows users to easily enable or disable debug output without modifying the source code.

This change improves flexibility for debugging while maintaining the existing functionality.

JustOscarJ1 avatar Jul 06 '24 00:07 JustOscarJ1

We'd have to update the read me too no? To debug what would the command be?

nadermx avatar Jul 08 '24 17:07 nadermx

I'd suggest using logging.basicConfig in the entrypoints instead of some random submodule, and only enable debug if the user passes a -v or --verbose flag, that way all of this StreamHandler can be removed

AFAIK the formatting can also be set in basicConfig, and instead of setting the level it can be passed directly in basicConfig.

lucasew avatar Apr 08 '25 14:04 lucasew

Thank you for this contribution! I appreciate your effort to improve debugging by adding proper logging. However, I'm closing this PR due to architectural concerns that were correctly identified by reviewers.

Why This Can't Be Merged As-Is

1. Violates Python Logging Best Practices

As @Tatsh correctly pointed out, handlers should only be added by end-users (at application entry points), not in library modules. Adding handlers at the module level:

  • Can interfere with users' logging configurations
  • Makes it difficult for library users to control logging output
  • Can lead to duplicate log messages
  • Prevents proper logging hierarchy management

See Python's logging documentation: https://docs.python.org/3/howto/logging.html#configuring-logging-for-a-library

2. Too Limited in Scope

This PR only changes one debug print statement. For a logging refactor to be worthwhile, it should:

  • Replace all print statements throughout the codebase with appropriate logging calls
  • Use proper log levels (DEBUG, INFO, WARNING, ERROR)
  • Provide consistent logging across all modules

3. Missing User Control

There's no way for users to control logging verbosity. A proper implementation should add command-line flags like --verbose or --debug.

How to Do This Properly

If you'd like to resubmit a proper logging implementation, here's what it should include:

1. In Each Module (No Handler Configuration!)

import logging

# Get a logger for this module - NO handler configuration!
logger = logging.getLogger(__name__)

# Replace print statements
logger.debug("path to be checked: %s", path)
logger.info("Processing video: %s", filename)
logger.warning("Model not found, downloading...")
logger.error("Failed to process: %s", error)

2. In Entry Points (cli.py, server.py)

import logging
import argparse

def main():
    ap = argparse.ArgumentParser()
    
    # Add logging flags
    ap.add_argument('--verbose', '-v', action='store_true',
                    help='Enable verbose output')
    ap.add_argument('--debug', action='store_true',
                    help='Enable debug output')
    
    args = ap.parse_args()
    
    # Configure logging ONLY at entry point
    log_level = logging.DEBUG if args.debug else (
        logging.INFO if args.verbose else logging.WARNING
    )
    
    logging.basicConfig(
        level=log_level,
        format='%(levelname)s: %(message)s'
    )
    
    # Now your code runs...

3. Replace ALL Print Statements

  • print(f"DEBUG: ...")logger.debug(...)
  • print(f"ERROR: ...")logger.error(...)
  • print("Starting ...")logger.info(...)
  • Error messages → logger.error(...) or logger.exception(...) for tracebacks

Resources

Would you be interested in submitting a more comprehensive PR that follows these guidelines? Happy to help review it!

nadermx avatar Oct 29 '25 00:10 nadermx