Update format parsing to support f-strings and the str.format function #7895

clavedeluna · 2022-12-03T16:31:30Z

Type of Changes

	Type
✓	🐛 Bug fix

Description

This PR takes over #6555 in hopes of getting the work merged.

Closes #6163
Closes #6085

DanielNoord · 2022-12-05T21:54:08Z

@clavedeluna Will you also work on fixing the tests? Otherwise it doens't make a lot of sense to keep this PR open.

clavedeluna · 2022-12-06T11:23:08Z

@clavedeluna Will you also work on fixing the tests? Otherwise it doens't make a lot of sense to keep this PR open.

Of course. What would be the point of this PR otherwise? The following work needs to happen:

add typing
fix tests

Typing is the first thing I think but I haven't had the energy to work on it much yet. If anyone is interested in contributing, please feel free to push commits.

DanielNoord · 2022-12-06T11:27:46Z

I'd focus on the tests first. The original PR never got the test suite to pass so there might be crucial incompatible changes in here. Typing of those change might become redundant after the incompatibilities have been fixed.

clavedeluna · 2022-12-27T16:03:36Z

I can now see why this was so difficult to get right in the first place. I did quite a bit but there are two tests that fail, and in trying to fix them so many other things break.

@DanielNoord maybe I won't be able to complete this, but I did find a couple functional tests that I'm not sure are right. In string_formatting.py:

"{0.missing.length}".format(ReturnYes())
 "{1.missing.length}".format(ReturnYes())

shouldn't those two lines emit some message, given that missing is not a real module?

DanielNoord · 2022-12-27T20:13:43Z

@clavedeluna Which messages were you thinking of?

clavedeluna · 2022-12-28T11:11:40Z

@clavedeluna Which messages were you thinking of?

Given this

"{0.missing}".format(2) # [missing-format-attribute]

I would expect

from missing import Missing
class ReturnYes:
    """ can't be properly inferred """
    missing = Missing()


"{0.missing.length}".format(ReturnYes())
"{1.missing.length}".format(ReturnYes())

to also raise missing-format-attribute. In the first example, 2 doesn't have a .missing attr. In the second group of cases, ReturnYes().missing.length also doesn't exit.

DanielNoord · 2023-01-02T11:13:43Z

Sorry for the late response, but I agree. That seems to make sense to me.

@clavedeluna

By @clavedeluna

clavedeluna · 2023-01-02T16:26:16Z

pylint/checkers/strings.py

+                if len(value.format_spec.values) == 0:
+                    continue
+
+                def convert_node_to_string(x):


move this func out for performance

clavedeluna · 2023-01-02T16:27:03Z

pylint/checkers/strings.py

+                and not arg_matches_format_type(arg_type, format_type)
+            ):
+                self.add_message(
+                    "bad-string-format-type",


try to dedupe with above code

clavedeluna · 2023-01-02T16:27:59Z

pylint/checkers/strings.py

@@ -634,6 +846,51 @@ def _check_new_format_specifiers(
                    # can't check further if we can't infer it
                    break

+        for position, specifiers in pos_args.items():


try to dedupe with above code

for more information, see https://pre-commit.ci

codecov · 2023-01-02T19:31:17Z

Codecov Report

Merging #7895 (a18894a) into main (0ac21c7) will decrease coverage by 0.50%.
The diff coverage is 84.52%.

❗ Current head a18894a differs from pull request most recent head daff78a. Consider uploading reports for the commit daff78a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7895      +/-   ##
==========================================
- Coverage   95.75%   95.25%   -0.50%     
==========================================
  Files         173      176       +3     
  Lines       18648    18751     +103     
==========================================
+ Hits        17856    17861       +5     
- Misses        792      890      +98

Files Changed	Coverage Δ
...int/checkers/refactoring/recommendation_checker.py	`96.35% <ø> (-0.19%)`	⬇️
pylint/checkers/strings.py	`90.09% <82.31%> (-4.06%)`	⬇️
pylint/checkers/utils.py	`94.51% <87.50%> (-1.40%)`	⬇️
pylint/checkers/logging.py	`94.70% <100.00%> (-0.04%)`	⬇️

... and 112 files with indirect coverage changes

clavedeluna · 2023-01-03T12:32:45Z

I'm thinking that the two primer reports are false positives but the one in music21 seems that it may be really hard to fix because :d is referring to the return from a function

Pierre-Sassoulas · 2023-01-03T13:25:21Z

I think for music21 bools can be considered as int too so it should not raise even without considering the function return, right ?

clavedeluna · 2023-02-07T11:51:35Z

I'm going to open this PR up for taking over since I just can't find the time to come back to it. Here is what I believe remains to be done:

fix primer results as per Pierre's comment above
fix pre-commit. this will require adding typing

for more information, see https://pre-commit.ci

github-actions · 2023-09-13T20:51:30Z

🤖 Effect of this PR on checked open source code: 🤖

Effect on music21:
The following messages are now emitted:

bad-string-format-type:
Argument 'builtins.bool' does not match format type 'd'
https://github.com/cuthbertLab/music21/blob/f457a2ba52ea3f978d805cd52fa101dfb0dd8577/music21/graph/axis.py#L1235

Effect on pytest:
The following messages are now emitted:

missing-function-docstring:
Missing function or method docstring
https://github.com/pytest-dev/pytest/blob/6c2feb75d2c4bb01aa145f8b85f7fb09fe4133cf/src/_pytest/capture.py#L408
missing-function-docstring:
Missing function or method docstring
https://github.com/pytest-dev/pytest/blob/6c2feb75d2c4bb01aa145f8b85f7fb09fe4133cf/src/_pytest/capture.py#L416

Effect on pandas:
The following messages are now emitted:

bad-format-string:
Invalid format string
https://github.com/pandas-dev/pandas/blob/79067a76adc448d17210f2cf4a858b0eb853be4c/pandas/tests/scalar/test_na_scalar.py#L38

Effect on sentry:
The following messages are now emitted:

bad-format-string:
Invalid format string
https://github.com/getsentry/sentry/blob/5f0659031453bff2a4505bf069ea1016e7269e96/src/sentry/notifications/notifications/rules.py#L217
bad-format-string:
Invalid format string
https://github.com/getsentry/sentry/blob/5f0659031453bff2a4505bf069ea1016e7269e96/src/sentry/notifications/notifications/digest.py#L112
bad-format-string:
Invalid format string
https://github.com/getsentry/sentry/blob/5f0659031453bff2a4505bf069ea1016e7269e96/src/sentry/web/frontend/debug/mail.py#L578
bad-format-string:
Invalid format string
https://github.com/getsentry/sentry/blob/5f0659031453bff2a4505bf069ea1016e7269e96/src/sentry/api/endpoints/project_transfer.py#L87

This comment was generated for commit daff78a

clavedeluna mentioned this pull request Dec 3, 2022

New format parsing #6555

Closed

This comment has been minimized.

Sign in to view

Pierre-Sassoulas added the Work in progress label Dec 4, 2022

clavedeluna force-pushed the format-parsing2 branch from 6fdaf9f to 39d23e3 Compare December 27, 2022 16:01

tmcarey and others added 6 commits January 2, 2023 10:40

Combine all code from PR pylint-dev#6555

fd73ebc

By @clavedeluna

add docs

421e3fe

try to fix pre-commit

2aaf764

fix some tests

095ba3b

add unit test

7f7ce67

explicit pos args as dict

ec868e6

clavedeluna force-pushed the format-parsing2 branch from ac4c5b2 to 2c5c9fe Compare January 2, 2023 16:24

clavedeluna commented Jan 2, 2023

View reviewed changes

fix functional test for dates

05f6e01

clavedeluna force-pushed the format-parsing2 branch from 3c60fce to 05f6e01 Compare January 2, 2023 19:18

[pre-commit.ci] auto fixes from pre-commit.com hooks

a18894a

for more information, see https://pre-commit.ci

This comment has been minimized.

Sign in to view

clavedeluna removed the Work in progress label Feb 7, 2023

clavedeluna added the Needs take over 🛎️ Orignal implementer went away but the code could be salvaged. label Feb 7, 2023

Pierre-Sassoulas and others added 2 commits September 13, 2023 22:22

Merge branch 'main' into format-parsing2

8491076

[pre-commit.ci] auto fixes from pre-commit.com hooks

daff78a

for more information, see https://pre-commit.ci

Pierre-Sassoulas changed the title ~~Format parsing~~ Update format parsing to support f-strings and the str.format function Sep 13, 2023

Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Sep 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update format parsing to support f-strings and the str.format function #7895

Update format parsing to support f-strings and the str.format function #7895

clavedeluna commented Dec 3, 2022

This comment has been minimized.

DanielNoord commented Dec 5, 2022

clavedeluna commented Dec 6, 2022

DanielNoord commented Dec 6, 2022

clavedeluna commented Dec 27, 2022

DanielNoord commented Dec 27, 2022

clavedeluna commented Dec 28, 2022

DanielNoord commented Jan 2, 2023

clavedeluna Jan 2, 2023

clavedeluna Jan 2, 2023

clavedeluna Jan 2, 2023

codecov bot commented Jan 2, 2023 •

edited

This comment has been minimized.

clavedeluna commented Jan 3, 2023

Pierre-Sassoulas commented Jan 3, 2023

clavedeluna commented Feb 7, 2023

github-actions bot commented Sep 13, 2023

Update format parsing to support f-strings and the str.format function #7895

Are you sure you want to change the base?

Update format parsing to support f-strings and the str.format function #7895

Conversation

clavedeluna commented Dec 3, 2022

Type of Changes

Description

This comment has been minimized.

DanielNoord commented Dec 5, 2022

clavedeluna commented Dec 6, 2022

DanielNoord commented Dec 6, 2022

clavedeluna commented Dec 27, 2022

DanielNoord commented Dec 27, 2022

clavedeluna commented Dec 28, 2022

DanielNoord commented Jan 2, 2023

clavedeluna Jan 2, 2023

Choose a reason for hiding this comment

clavedeluna Jan 2, 2023

Choose a reason for hiding this comment

clavedeluna Jan 2, 2023

Choose a reason for hiding this comment

codecov bot commented Jan 2, 2023 • edited

Codecov Report

This comment has been minimized.

clavedeluna commented Jan 3, 2023

Pierre-Sassoulas commented Jan 3, 2023

clavedeluna commented Feb 7, 2023

github-actions bot commented Sep 13, 2023

codecov bot commented Jan 2, 2023 •

edited