[ruff] if k in d: del d[k] (RUF051)

[InSyncWithFoo]

Summary

Resolves #7537.

Test Plan

cargo nextest run and cargo insta test.

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+16 -0 violations, +0 -0 fixes in 6 projects; 49 projects unchanged)

DisnakeDev/disnake (+3 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ disnake/ext/commands/cog.py:177:21: RUF051 [*] Use `pop` instead of `key in dict` followed by `delete dict[key]`
+ disnake/ext/commands/cog.py:179:21: RUF051 [*] Use `pop` instead of `key in dict` followed by `delete dict[key]`
+ disnake/ext/commands/cog.py:181:21: RUF051 [*] Use `pop` instead of `key in dict` followed by `delete dict[key]`

RasaHQ/rasa (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ rasa/core/migrate.py:134:9: RUF051 [*] Use `pop` instead of `key in dict` followed by `delete dict[key]`
+ rasa/nlu/classifiers/diet_classifier.py:781:13: RUF051 [*] Use `pop` instead of `key in dict` followed by `delete dict[key]`

apache/airflow (+4 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ docs/build_docs.py:604:13: RUF051 [*] Use `pop` instead of `key in dict` followed by `delete dict[key]`
+ docs/build_docs.py:606:13: RUF051 [*] Use `pop` instead of `key in dict` followed by `delete dict[key]`
+ providers/src/airflow/providers/google/cloud/operators/dataproc.py:666:17: RUF051 [*] Use `pop` instead of `key in dict` followed by `delete dict[key]`
+ providers/src/airflow/providers/google/cloud/operators/dataproc.py:680:21: RUF051 [*] Use `pop` instead of `key in dict` followed by `delete dict[key]`

apache/superset (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ superset/dashboards/schemas.py:185:13: RUF051 [*] Use `pop` instead of `key in dict` followed by `delete dict[key]`

zulip/zulip (+5 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ corporate/tests/test_stripe.py:662:17: RUF051 [*] Use `pop` instead of `key in dict` followed by `delete dict[key]`
+ zerver/lib/markdown/__init__.py:2456:9: RUF051 [*] Use `pop` instead of `key in dict` followed by `delete dict[key]`
+ zerver/lib/test_runner.py:103:13: RUF051 [*] Use `pop` instead of `key in dict` followed by `delete dict[key]`
+ zerver/tornado/event_queue.py:542:13: RUF051 [*] Use `pop` instead of `key in dict` followed by `delete dict[key]`
+ zerver/tornado/handlers.py:42:9: RUF051 [*] Use `pop` instead of `key in dict` followed by `delete dict[key]`

astropy/astropy (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ astropy/modeling/core.py:189:17: RUF051 [*] Use `pop` instead of `key in dict` followed by `delete dict[key]`

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
RUF051	16	16	0	0	0

[MichaReiser]

Thanks.

This rule would fit well into the flake8-simplify group. Would you be interested in creating an upstream issue to see if they're interested in such a rule?

[InSyncWithFoo]

Created upstream issue here. The repo has been inactive for nearly a year, so I won't hold my breath.

[MichaReiser]

Thanks for opening the upstream issue.

We discussed this internally. Let's give flake8-simplify about a week to get back to the issue. If we don't hear back in a week, then let's implement this rule as part of ruf (as you did).

@InSyncWithFoo feel free to ping me after a week

[InSyncWithFoo]

@MichaReiser It has been a week. What do you say?

[MichaReiser]

Thanks. I left a few comments related to the code.

The main question to me is if we want to add this rule to Ruff today. I see that @charliermarsh mark the rule as accepted.

To me, this seems to be mainly a stylistic rule and I personally would prefer if x in k: del k[x] in many places. But maybe that's just because I'm unused to a dict type to not have a remove method. I'd be on board merging this rule if we consider using pop is a more idiomatic writting of this pattern. @AlexWaygood what's your take on this?

[AlexWaygood]

To me, this seems to be mainly a stylistic rule and I personally would prefer if x in k: del k[x] in many places. But maybe that's just because I'm unused to a dict type to not have a remove method. I'd be on board merging this rule if we consider using pop is a more idiomatic writting of this pattern. @AlexWaygood what's your take on this?

I agree that using .pop(..., None) is the idiomatic way to do this in Python. The rule makes sense to me. While I agree that it reads a little bit awkwardly if you're new to the language, it's much more efficient than if x in k: del k[x], and it's more concise. I feel like pointing out better idioms in a certain language that may not exist in other languages is one of the most helpful things linters can do for beginners in that language. So I support the rule.

[MichaReiser]

Perfect, thanks @AlexWaygood for the extra context (and educating me :))

I'm requesting changes for the code review feedback that I submitted earlier today