ruff

ruff copied to clipboard

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

May 11 '24 17:05 hassec

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)

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__`

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__`

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__`

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__`

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__`

May 11 '24 17:05 github-actions[bot]

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?

May 16 '24 02:05 charliermarsh

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?

May 16 '24 02:05 charliermarsh

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

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

May 16 '24 02:05 hassec

@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.

May 26 '24 00:05 hassec

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()

May 26 '24 14:05 zanieb

@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?

May 30 '24 00:05 hassec

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).

May 30 '24 00:05 charliermarsh

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

May 30 '24 03:05 codspeed-hq[bot]

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 (https://github.com/astral-sh/ruff/pull/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 ^^"

Jun 01 '24 17:06 Avasam

Thanks, good call!

Jun 01 '24 18:06 charliermarsh