Inconsistent logging detection

[konstin]

There are three ways you can call the logging module:

import logging
logger = logging.getLoger(__name__)
logger.info("msg")

import logging
logging.info("msg")

from logging import info
info("msg")

The logging rules are enforced inconsistently between those three call kinds:

import logging
from logging import warn, info, error

logger = logging.getLogger(__name__)

logging.info(f"{1}")  # G004
logger.info(f"{1}")  # G004
info(f"{1}")

logging.info("", 2)  # PLE1205
logger.info("", 2)  # PLE1205
info("", 2)

logging.warn("a")  # PGH002, G010
logger.warn("a")  # G010
warn("a")  # PGH002

try:
    print(1 / 0)
except Exception:
    logging.error("a")  # TRY400
    logger.error("a")  # TRY400
    error("a")

Checking for a common logger name is generally consistent, but PGH002 mismatches expectations here.

It might also make sense to look for usages of logging.getLogger for a more semantic coverage, even though the vast majority of usages in the wild seem to match our pattern (see also https://grep.app/search?q=logging.getLogger&filter[lang][0]=Python, https://github.com/hummingbot/hummingbot/blob/b3ba248e7b9bab26c010fcb69a53040146084adb/hummingbot/client/config/conf_migration.py#L45, https://github.com/qutebrowser/qutebrowser/blob/4190a470c56ed90d97af89711a30c4603a347858/qutebrowser/utils/log.py#L104)

The main questions i'm trying to answer are, a) should we change the scope of the existing rule (or not) and b) which of the three cases should new logging rules capture?

[charliermarsh]

We should be treating error as equivalent to logging.error -- that seems like a bug in is_logger_candidate.

And we should change PGH002 to match G010. Actually, we should probably just remove PGH002, and redirect to G010?

[charliermarsh]

We should be treating error as equivalent to logging.error

I looked into this a bit and I think it requires some minor modifications to the various rules that call is_logger_candidate.

[qdegraaf]

I looked into this a bit and I think it requires some minor modifications to the various rules that call is_logger_candidate.

Could it not be handled in is_logger_candidate() itself?

Am I correct in thinking the issue is that is_logger_candidate() assumes the func to be an Expr::Attribute whereas a loose error() call is an Expr::Name? If so, would it be better to add a branch of logic to is_logger_candidate for this scenario, seeing as resolve_call_path() and collect_call_path() already allow for both Expr::Attribute and Expr::Name values to be passed?

[charliermarsh]

Closed by #7521.