mirror of
https://github.com/astral-sh/ruff
synced 2026-01-21 21:40:51 -05:00
15aa74206e
Summary -- This PR fixes #14900 by: - Restricting the diagnostic range from the whole `for` loop to only the `target in iter` part - Adding secondary annotations to each use of the `dict[key]` accesses - Adding a `fix_title` suggesting to use `for key in dict.items()` I thought this approach sounded slightly nicer than the alternative of renaming the rule to focus on each indexing operation mentioned in https://github.com/astral-sh/ruff/issues/14900#issuecomment-2543923625, but I don't feel too strongly. This was easy to implement with our new diagnostic infrastructure too. This produces an example annotation like this: ``` PLC0206 Extracting value from dictionary without calling `.items()` --> dict_index_missing_items.py:59:5 | 58 | # A case with multiple uses of the value to show off the secondary annotations 59 | for instrument in ORCHESTRA: | ^^^^^^^^^^^^^^^^^^^^^^^ 60 | data = json.dumps( 61 | { 62 | "instrument": instrument, 63 | "section": ORCHESTRA[instrument], | --------------------- 64 | } 65 | ) 66 | 67 | print(f"saving data for {instrument} in {ORCHESTRA[instrument]}") | --------------------- 68 | 69 | with open(f"{instrument}/{ORCHESTRA[instrument]}.txt", "w") as f: | --------------------- 70 | f.write(data) | help: Use `for instrument, value in ORCHESTRA.items()` instead ``` which I think is a big improvement over: ``` PLC0206 Extracting value from dictionary without calling `.items()` --> dict_index_missing_items.py:59:1 | 58 | # A case with multiple uses of the value to show off the secondary annotations 59 | / for instrument in ORCHESTRA: 60 | | data = json.dumps( 61 | | { 62 | | "instrument": instrument, 63 | | "section": ORCHESTRA[instrument], 64 | | } 65 | | ) 66 | | 67 | | print(f"saving data for {instrument} in {ORCHESTRA[instrument]}") 68 | | 69 | | with open(f"{instrument}/{ORCHESTRA[instrument]}.txt", "w") as f: 70 | | f.write(data) | |_____________________^ | ``` The secondary annotation feels a bit bare without a message, but I thought it might be too busy to include one. Something like `value extracted here` or `indexed here` might work if we do want to include a brief message. To avoid collecting a `Vec` of annotation ranges, I added a `&Checker` to the rule's visitor to emit diagnostics as we go instead of at the end. Test Plan -- Existing tests, plus a new case showing off multiple secondary annotations