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

fix(F822): add option to enable F822 in __init__.py files #11370

Merged
merged 2 commits into from
May 30, 2024

Conversation

hassec
Copy link
Contributor

@hassec hassec commented May 11, 2024

Summary

This PR aims to close #10095 by adding an option init-allow-undef-export to the pyflakes settings. This option is currently set to true such that behavior is kept identical.
But setting this option to false will lead to F822 warnings to be shown in all files, including __init__.py files.

As I've mentioned on #10095, I think init-allow-undef-export=false would be the more user-friendly default option, as it creates fewer surprises. @charliermarsh what do you think about making that the default?

With this option in place, it's a single line fix for people that rely on the old behavior.

And thinking longer term, for future major releases, one could probably consider deprecating the option and eventually having people just noqa these warnings if they are not wanted.

Test Plan

I've added a test_init_f822_enabled test which repeats the test that is done in the init test but this time with init-allow-undef-export=false and the snap file correctly shows that ruff will then trigger the otherwise suppressed F822 warning.

closes #10095

Copy link
Contributor

github-actions bot commented May 11, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

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

PostHog/HouseWatch (+1 -0 violations, +0 -0 fixes)

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

+ housewatch/tasks/__init__.py:5:43: F822 Undefined name `customer_report` in `__all__`

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

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

+ airflow/providers/cncf/kubernetes/utils/__init__.py:19:12: F822 Undefined name `xcom_sidecar` in `__all__`
+ airflow/providers/cncf/kubernetes/utils/__init__.py:19:28: F822 Undefined name `pod_manager` in `__all__`

mlflow/mlflow (+4 -0 violations, +0 -0 fixes)

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

+ mlflow/metrics/__init__.py:444:5: F822 Undefined name `accuracy` in `__all__`
+ mlflow/metrics/__init__.py:456:5: F822 Undefined name `binary_recall` in `__all__`
+ mlflow/metrics/__init__.py:457:5: F822 Undefined name `binary_precision` in `__all__`
+ mlflow/metrics/__init__.py:458:5: F822 Undefined name `binary_f1_score` in `__all__`

prefecthq/prefect (+3 -0 violations, +0 -0 fixes)

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

+ src/prefect/blocks/__init__.py:7:12: F822 Undefined name `notifications` in `__all__`
+ src/prefect/blocks/__init__.py:7:29: F822 Undefined name `system` in `__all__`
+ src/prefect/blocks/__init__.py:7:39: F822 Undefined name `webhook` in `__all__`

indico/indico (+2 -0 violations, +0 -0 fixes)

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

+ indico/modules/api/__init__.py:20:12: F822 Undefined name `settings` in `__all__`
+ indico/modules/logs/__init__.py:19:60: F822 Undefined name `CategoryLogRealm` in `__all__`

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
F822 12 12 0 0 0

@charliermarsh
Copy link
Member

I'm wondering if we should instead always enable this, and ask users to add F822 to per-file-ignores if they want it disabled in that context. Could you try always enabling this, and see if the ecosystem checks surface any violations?

@charliermarsh
Copy link
Member

I.e., if we think tis should be the default, maybe we just make it the default and use the existing per-file-ignores mechanism for those that need to turn it off?

@hassec hassec force-pushed the enable_f822_in_init branch 2 times, most recently from 621369e to 31f6f9b Compare May 16, 2024 02:29
@hassec
Copy link
Contributor Author

hassec commented May 16, 2024

Thanks for taking a look at this @charliermarsh.
I like your suggestion, I'd be happy to have this be the default 😊

I've changed the PR to simply remove the special handling of F822 warnings from __init__.py files.

@hassec
Copy link
Contributor Author

hassec commented May 26, 2024

@charliermarsh just a gentle reminder 😊
To me, the ecosystem checks actually show that it would be beneficial to make this the default, and I bet most people assume that their __init__.py files are getting checked already.
And I like your suggestion of just having people use per-file-ignore for the few cases where it would be a false positive.

@zanieb
Copy link
Member

zanieb commented May 26, 2024

Could you make this change only apply when preview mode is enabled so we don't increase the scope of the rule without a stabilization period? I think it'd be checker.settings.preview.is_enabled()

@hassec
Copy link
Contributor Author

hassec commented May 30, 2024

@zanieb apologies for the slow reply, it's been a busy work week so far.

I've made the suggested change and added a test for the preview to make sure the preview enabled scenario is tested too.

How long do changes like this usually have to be in preview, before they can become stable?

@charliermarsh
Copy link
Member

Thanks @hassec. Will take a look shortly. We tend to leave a change in preview for one minor release (so this would be elevated in v0.6.0, since v0.5.0 will go out in the next few weeks).

@charliermarsh charliermarsh added the preview Related to preview mode features label May 30, 2024
@charliermarsh charliermarsh enabled auto-merge (squash) May 30, 2024 03:11
@charliermarsh charliermarsh merged commit e35deee into astral-sh:main May 30, 2024
17 checks passed
Copy link

codspeed-hq bot commented May 30, 2024

CodSpeed Performance Report

Merging #11370 will improve performances by 7.9%

Comparing hassec:enable_f822_in_init (164d4cf) with hassec:enable_f822_in_init (686ef15)

Summary

⚡ 1 improvements
✅ 29 untouched benchmarks

Benchmarks breakdown

Benchmark hassec:enable_f822_in_init hassec:enable_f822_in_init Change
parser[numpy/ctypeslib.py] 1.2 ms 1.1 ms +7.9%

@Avasam
Copy link

Avasam commented Jun 1, 2024

Hey thanks for the change! Just a nitpick as a user about the changelog entry, it says

[pyflakes] Add option to enable F822 in init.py files (#11370)

So I went looking for the new option, even reading this PR's description and looking for init-allow-undef-export, but turns out this is just enabled by default, with an appropriate lint.per-file-ignores recommendation to disable ^^"

@charliermarsh
Copy link
Member

Thanks, good call!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

F822 Undefined name in __all__ does not work in __init__.py
4 participants