False positive on G003?

[steven-murray]

A statement like logger.info("This is the {i+1}th iteration") raises G003 but this is clearly not a concatenation.

[sethisernhagen]

Hi @steven-murray. I tried to repro the issue here without any luck. https://github.com/globality-corp/flake8-logging-format/blob/feature/string-concat-bug/logging_format/tests/test_visitor.py#L388-L398

[steven-murray]

Interesting. My actual line is

logger.info(f"Simulating Frequency {i+1}/{len(data_model.freqs)}")

but I can't see anything much different in this than the one in the test. In case it matters, my pre-commit setup is:

-   repo: https://github.com/PyCQA/flake8
    rev: 5.0.4  # pick a git hash / tag to point to
    hooks:
    -   id: flake8
        additional_dependencies:
          - flake8-rst-docstrings
          - flake8-builtins
          - flake8-logging-format
          - flake8-rst-docstrings
          - flake8-rst
          - flake8-bugbear
          - flake8-comprehensions
          - flake8-print

[sethisernhagen]

Updated the test. Now I see some violations.

E       AssertionError: 
E       Expected: an object with length of <0>
E            but: was <[(<_ast.JoinedStr object at 0x7f5e1b9b3150>, 'G004 Logging statement uses f-string'), (<_ast.BinOp object at 0x7f5e1b9b30d0>, "G003 Logging statement uses '+'")]> with length of <2>

G004 seems like a legit violation. G003 would go away if not using a f-string.

Isn't f"Simulating Frequency {i+1}/{len(data_model.freqs)}" concatenating string values ?

[steven-murray]

Hmmm. So I am ignoring G004 because of this issue: #32. I don't think f"Simulating Frequency {i+1}/{len(data_model.freqs)}" is concatenating anything, it's just string interpolation. And the "+" is certainly not there for concatenation -- it's there to add integers!

[sethisernhagen]

I see what you are saying now. The G003 is a false positive.

This plugin tries to enforce static log message values. Any non-static values are meant to be added to the extra dict. This way the static message value can be used as a unique identifier for the event and all non-static data is nicely contained with the extra dict. Does that make sense?

The readme is confusing I think because it's missing info. You need a to use a log formatter to do what it is trying to explain (magically updating message values with the extra dict key/values). Here is a formatter that does this https://github.com/globality-corp/microcosm-logging/blob/develop/microcosm_logging/formatters.py#L11

You can use this ExtraConsoleFormatter when doing local development as you probably don't care about the non-static message. When you want your logs ingested by a log aggregator you could use python-json-logger formatter to output nice json logs with the static message identifier and extra key/values.

[steven-murray]

I think I understand the motivation for doing so, but I don't feel that it should be considered bad style to use non-static message values in logging statement, especially given that you need 3rd-party tools to enable it! So, it seems if I want some of the functionality of this plugin, but without this the G003, I should then disable both G003 and G004?