No single letter variables

[klssmith]

This allows _ to be used as a variable name, since it is conventionally
used for throwaway variables, but does not allow any other single letter
variables.

The target method of ast.Assigns will normally contain a list of nodes,
however for an unpacking assignment (such as 'a,b = c') it will contain
an ast.Tuple or an ast.List so we need to cover for all cases.

[klssmith]

@MarcoGorelli This is giving an error with the mypy precommit hook (error: "expr" has no attribute "id” on line 22 of single_letter_variables.py) which I can’t figure out. It’s also more complicated than originally discussed to cover for a few edge cases, so I’m not sure if this is still along the right lines or not.

[codecov]

Codecov Report

Merging #25 (1c6f213) into main (92aff77) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #25   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           47        49    +2     
  Lines          633       659   +26     
  Branches        80        84    +4     
=========================================
+ Hits           633       659   +26     

Impacted Files	Coverage Δ
tests/string_to_concatenate_test.py	100.00% <ø> (ø)
...ev_flaker/_plugins_tree/single_letter_variables.py	100.00% <100.00%> (ø)
tests/single_letter_variables_test.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92aff77...1c6f213. Read the comment docs.

[MarcoGorelli]

Awesome work, I'm really impressed you figured this all out!

For the mypy issue, we just need to "help" mypy and tell it that the target elements are ast.Names, and hence that they have the id attribute. You probably want to write it like this:

    for item in assignment_names:
        if isinstance(item, ast.Name) and item.id != "_" and len(item.id) == 1:
            yield node.lineno, node.col_offset, MSG

cc @jbrockmendel in case you have comments - this'll raise an error all over the place in pandas, but it can be fixed incrementally

[jbrockmendel]

This is pretty neat. _ makes sense to special-case. Maybe also e.g. for i in range(...)? (i know we do that a lot in the cython code)

[MarcoGorelli]

Thanks for taking a look - for i in range wouldn't be covered by this, as that's ast.For rather than ast.Assign

@klssmith if you mark this as "ready for review" we can merge it

[klssmith]

Thanks for the comments.

@MarcoGorelli I've made the change needed to pass the mypy precommit check. I've not changed this to ready to review just yet because I've noticed that the col_offset in the error message can be misleading in a few situations. A file with this line

abc, d = (5, 6)

gives the warning of

test.py:1:1: PDF023 found assignment to single-letter variable

Since the single letter variable is at col_offset 6, I assume the message should read test.py:1:6: PDF023 ... in this case? If so, I can take a look at how the col_offset in error message can be fixed.

[MarcoGorelli]

when you're in the for loop, does item have .lineno and coloffset attributes? If so, perhaps use those in the message?

[klssmith]

👍 I was able to replace yield node.lineno, node.col_offset, MSG with yield item.lineno, item.col_offset, MSG as you suggested to get the col_offset working.

[MarcoGorelli]

Awesome work! Thanks @klssmith !

If you enjoyed this kind of work (ast-parsing), I'd suggest you have a look at pyupgrade too - their maintainer is amazing, I've learned a ton from contributing there

[klssmith]

Thanks for the help @MarcoGorelli! I will check out pyupgrade