Only parse format strings when being used with locals()

jingw commented 4 years ago

Description

The previous behavior has confusing results when strings aren't format strings, e.g. docstrings that happen to have a fragment resembling a format string. The new behavior is more precise.

The downside is that this won't detect usages when the format string or locals() is assigned to an intermediate variable before formatting. That seems fine to me, since it surprises me that the code x = "{y}" counts as usage of y. One could also argue against handling this at all, given that locals() is somewhat magical and that f-strings are the future.

Related Issue

Haven't filed one. Happy to discuss.

Checklist:

[x] My code follows the code style of this project.
(N/A) I have updated the documentation in the README.md file.
[x] I have added an entry in CHANGELOG.md.
[x] I have added tests to cover my changes.
[x] All new and existing tests passed.

jingw commented 4 years ago

I'm not 100% sure what's up with the failing test. It's complaining that a file should be reformatted, but on a file I didn't touch.

style run-test: commands[0] | black --check --diff .
--- tests/test_config.py    2020-08-27 01:02:40.913556 +0000
+++ tests/test_config.py    2020-08-27 01:03:08.744364 +0000
@@ -155,11 +155,12 @@
     with pytest.raises(SystemExit):
         _check_input_config({"unknown_key_1": 1})

 @pytest.mark.parametrize(
-    "key, value", list(DEFAULTS.items()),
+    "key, value",
+    list(DEFAULTS.items()),
 )
 def test_incompatible_option_type(key, value):
     """
     If a config value has a different type from the default value we abort.
     """
would reformat tests/test_config.py
Oh no! 💥 💔 💥
1 file would be reformatted, 38 files would be left unchanged.

There appears to be something going on with black==20.8b1 versus 19.10b0. Version 20.8b1 was apparently just released. I don't see anything pinning a black version, but I also don't see why tox is giving me 19.10b0 after nuking the virtualenv and recreating it.

jingw commented 4 years ago

Went ahead and appeased black's behavior change, stepping around "support for explicit trailing commas" :shrug:

jendrikseipp commented 4 years ago

Thanks for the patch!

I'm not sure how to handle things like the black error in the future. Would you recommend pinning the version for linters like black and flake8 in the tox file?

jingw commented 4 years ago

Both options are annoying IMO. The status quo makes CI break when something changes in a dependency, whereas pinning leaves you with stale versions unless you manually upgrade stuff periodically. (For personal projects, I don't bother pinning and just accept occasional breakages. For my job, breaking CI is disruptive, so everything's pinned.)

jendrikseipp commented 4 years ago

I agree. I'll leave things as they are for now. Once dependabot supports tox.ini files, I'll probably try using it to create PRs for bumping versions automatically.

jendrikseipp / vulture