Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jan 8, 2024

Conversation

charliermarsh
Copy link
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
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
Member Author

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

Copy link
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 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
Member Author

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

@charliermarsh
Copy link
Member Author

Perhaps this should be preview though.

Copy link
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
17 checks passed
@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.

None yet

2 participants