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

New Rule: Formatted string passed to logging module PYL-W1203 #11854

Open
Zerotask opened this issue Jun 13, 2024 · 7 comments
Open

New Rule: Formatted string passed to logging module PYL-W1203 #11854

Zerotask opened this issue Jun 13, 2024 · 7 comments
Labels
needs-info More information is needed from the issue author

Comments

@Zerotask
Copy link

Zerotask commented Jun 13, 2024

Deepsource warns you, when you use a formatted string with the logging module.

Formatting the message manually before passing it to a logging call does unnecessary work if logging is disabled. Consider using the logging module's built-in formatting features to avoid that.

Example:

msg = "test"
logger.error(f"{msg} failed")

and suggest to use it with %like so:

msg = "test"
logger.error("%s failed", msg)
@matteo-zanoni
Copy link

I'm new to working with ruff but I would like to tackle this.
It looks similar enough to PLE1205 and PLE1206 that are already implemented an look like a good starting place.
Is this new rule something that is approved? Can I start working on it?
Would appreciate any further guidance.

@tdulcet
Copy link

tdulcet commented Jun 13, 2024

This is already implemented as part of G001, G002 and G004. What is still needed though is an autofix, as we have hundreds of these violations and they are very tedious to fix manually.

@Zerotask
Copy link
Author

This is already implemented as part of G001, G002 and G004. What is still needed though is an autofix, as we have hundreds of these violations and they are very tedious to fix manually.

We're using the "G" rule and ruff does not detect this.

@charliermarsh
Copy link
Member

I see it raised here: https://play.ruff.rs/4e92508d-36ee-476b-b67d-7bc6d8c1e457

@charliermarsh charliermarsh added the needs-info More information is needed from the issue author label Jun 13, 2024
@Zerotask
Copy link
Author

I see it raised here: https://play.ruff.rs/4e92508d-36ee-476b-b67d-7bc6d8c1e457

It seems that the error fades, when I use for example import logger. In our case we create a logger instance in another module and just import it then.
If I create the logger in the same module, the error will be shown.

@dhruvmanila
Copy link
Member

It seems that the error fades, when I use for example import logger. In our case we create a logger instance in another module and just import it then. If I create the logger in the same module, the error will be shown.

Yes, that's currently a limitation of Ruff where it doesn't know the context from other modules. Currently, Ruff only looks at one file at a time but that's going to be changed in the future where we add support for multifile analysis.

@matteo-zanoni
Copy link

Actually it looks like those errors are either raised or not based only on the name of the object calling the error, info, etc... function.

So for example:

msg = "test"

class Foo:
    def __init__(self):
        ...

    def error(self, msg: str):
        raise ValueError(msg)

log = Foo()
log.error(f"{msg} failed")

this raises the error but

msg = "test"

class Foo:
    def __init__(self):
        ...

    def error(self, msg: str):
        raise ValueError(msg)

foo = Foo()
foo.error(f"{msg} failed")

this does not.
In this case neither should be an error (since they are not calls to the logging python module), but the same holds for instances where the error should be raised:

This shows an error

import logging

msg = "test"
logger = logging.getLogger("foo")
logger.error(f"{msg} failed")

But this does not

import logging

msg = "test"
foo = logging.getLogger("foo")
foo.error(f"{msg} failed")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-info More information is needed from the issue author
Projects
None yet
Development

No branches or pull requests

5 participants