Skip to content

Don't flag redefined-while-unused in if branches#9418

Merged
charliermarsh merged 1 commit intomainfrom
charlie/same-branch
Jan 8, 2024
Merged

Don't flag redefined-while-unused in if branches#9418
charliermarsh merged 1 commit intomainfrom
charlie/same-branch

Conversation

@charliermarsh
Copy link
Copy Markdown
Member

@charliermarsh charliermarsh commented Jan 7, 2024

Summary

On main, we flag redefinitions in cases like:

import os

x = 1

if x > 0:
    import os

That is, we consider these to be in the "same branch", since they're not in disjoint branches. This matches Flake8's behavior, but it seems to lead to false positives.

@charliermarsh charliermarsh changed the title Don't flag redefined-while-unsed in if branches Don't flag redefined-while-unsed in if branches Jan 7, 2024
@charliermarsh
Copy link
Copy Markdown
Member Author

Just looking at # noqa: F811 usages in Code Search... At present, we often flag redefinitions within conditionals, and I don't see why we should:

with safe_import() as exc:
    import cffi
if exc.error:
    cffi = None  # noqa: F811
from kivy.core.window import Window
from kivy.metrics import dp
from kivy.utils import platform

if "KIVY_DOC_INCLUDE" in os.environ:
    dp = lambda x: x  # NOQA: F811

@charliermarsh
Copy link
Copy Markdown
Member Author

Enforcing this is also the source of the bug we introduced in #2044.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jan 7, 2024

ruff-ecosystem results

Linter (stable)

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

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

ruff check --no-cache --exit-zero --no-preview --select ALL

+ tests/www/views/test_views_rendered.py:259:55: RUF100 [*] Unused `noqa` directive (unused: `F811`)

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

ruff check --no-cache --exit-zero --no-preview --select ALL

+ src/bokeh/embed/util.py:143:32: RUF100 [*] Unused blanket `noqa` directive

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
RUF100 2 2 0 0 0

Linter (preview)

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

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

ruff check --no-cache --exit-zero --preview --select ALL

+ tests/www/views/test_views_rendered.py:259:55: RUF100 [*] Unused `noqa` directive (unused: `F811`)

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

ruff check --no-cache --exit-zero --preview --select ALL

+ src/bokeh/embed/util.py:143:32: RUF100 [*] Unused blanket `noqa` directive

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
RUF100 2 2 0 0 0

@charliermarsh charliermarsh requested a review from zanieb January 7, 2024 01:55
@charliermarsh charliermarsh changed the title Don't flag redefined-while-unsed in if branches Don't flag redefined-while-unused in if branches Jan 7, 2024
@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Jan 7, 2024
@charliermarsh
Copy link
Copy Markdown
Member Author

Enforcing this would also resolve the bug introduced in #5561 (comment).

@charliermarsh
Copy link
Copy Markdown
Member Author

Perhaps this should be preview though.

Copy link
Copy Markdown
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is okay without preview since it's a reduction in scope; unless you think it is possible we will revert.

@charliermarsh charliermarsh merged commit 985f1d1 into main Jan 8, 2024
@charliermarsh charliermarsh deleted the charlie/same-branch branch January 8, 2024 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rule Implementing or modifying a lint rule

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants