Open steven-murray opened 1 year ago
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
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
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 ?
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!
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.
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?
A statement like
logger.info("This is the {i+1}th iteration")
raises G003 but this is clearly not a concatenation.