Extend `invalid-envvar-default (PLW1508)` to detect `os.environ.get` by harupy · Pull Request #14512 · astral-sh/ruff

harupy · 2024-11-21T11:24:55Z

Summary

Fix invalid-envvar-default (PLW1508) to flag os.environ.get.

import os


os.getenv("a", 1)   # PLW1508
os.environ.get("a", 1)  # no PLW1508

Test Plan

New test case

github-actions · 2024-11-21T11:31:13Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

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

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

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

+ airflow/api_fastapi/core_api/app.py:55:43: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ providers/src/airflow/providers/google/marketing_platform/example_dags/example_display_video.py:49:53: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ providers/src/airflow/providers/google/marketing_platform/example_dags/example_display_video.py:56:51: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ providers/tests/system/amazon/aws/example_bedrock.py:58:70: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ providers/tests/system/amazon/aws/example_bedrock.py:63:86: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ providers/tests/system/google/cloud/storage_transfer/example_cloud_storage_transfer_service_aws.py:77:91: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ scripts/in_container/update_quarantined_test_status.py:208:47: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ scripts/in_container/update_quarantined_test_status.py:209:47: PLW1508 Invalid type for environment variable default; expected `str` or `None`

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

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

+ superset/config.py:1579:77: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ superset/migrations/shared/utils.py:35:55: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ superset/migrations/versions/2020-09-28_17-57_b56500de1855_add_uuid_column_to_import_mixin.py:76:55: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ superset/migrations/versions/2020-10-21_21-09_96e99fb176a0_add_import_mixing_to_saved_query.py:54:55: PLW1508 Invalid type for environment variable default; expected `str` or `None`

aws/aws-sam-cli (+12 -0 violations, +0 -0 fixes)

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

+ samcli/local/docker/container.py:41:93: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ samcli/local/lambdafn/remote_files.py:33:93: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ tests/integration/buildcmd/test_build_terraform_applications.py:177:69: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ tests/integration/package/package_integ_base.py:51:77: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ tests/integration/package/package_integ_base.py:55:78: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ tests/integration/publish/publish_app_integ_base.py:22:75: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ tests/regression/package/regression_package_base.py:24:75: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ tests/testing_utils.py:24:50: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ tests/testing_utils.py:26:50: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ tests/testing_utils.py:31:54: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ tests/testing_utils.py:31:94: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ tests/testing_utils.py:32:45: PLW1508 Invalid type for environment variable default; expected `str` or `None`

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

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

+ src/bokeh/settings.py:338:53: PLW1508 Invalid type for environment variable default; expected `str` or `None`

fronzbot/blinkpy (+1 -0 violations, +0 -0 fixes)

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

+ blinkapp/blinkapp.py:12:48: PLW1508 Invalid type for environment variable default; expected `str` or `None`

ibis-project/ibis (+11 -0 violations, +0 -0 fixes)

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

+ ibis/backends/clickhouse/tests/conftest.py:22:67: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ ibis/backends/clickhouse/tests/test_client.py:221:62: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ ibis/backends/clickhouse/tests/test_client.py:376:60: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ ibis/backends/exasol/tests/conftest.py:22:59: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ ibis/backends/mssql/tests/conftest.py:18:57: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ ibis/backends/mysql/tests/conftest.py:19:57: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ ibis/backends/oracle/tests/conftest.py:23:59: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ ibis/backends/postgres/tests/conftest.py:37:78: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ ibis/backends/risingwave/tests/conftest.py:22:80: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ ibis/backends/trino/tests/conftest.py:31:73: PLW1508 Invalid type for environment variable default; expected `str` or `None`
+ ibis/expr/tests/test_visualize.py:19:39: PLW1508 Invalid type for environment variable default; expected `str` or `None`

latchbio/latch (+1 -0 violations, +0 -0 fixes)

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

+ src/latch_cli/snakemake/single_task_snakemake.py:38:63: PLW1508 Invalid type for environment variable default; expected `str` or `None`

pandas-dev/pandas (+1 -0 violations, +0 -0 fixes)

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

+ setup.py:367:54: PLW1508 Invalid type for environment variable default; expected `str` or `None`

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

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

+ rotkehlchen/tests/fixtures/google.py:66:69: PLW1508 Invalid type for environment variable default; expected `str` or `None`

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
PLW1508	40	40	0	0	0

harupy · 2024-11-21T11:40:30Z

Should I update the doc to mention both os.environ.get and os.getenv?

MichaReiser · 2024-11-21T12:27:17Z

Is this a change to align the rule with the upstream rule?

I think we should gate this behind preview, considering that it extends the scope of the rule (even though it is in the rule's intent)

harupy · 2024-11-21T12:32:05Z

Is this a change to align the rule with the upstream rule?

No, pylint only checks os.getenv:

https://github.com/pylint-dev/pylint/blob/68cb5b320653ad64c68ff48a4bb4ba449a01d3a6/pylint/checkers/stdlib.py#L33

I think we should gate this behind preview, considering that it extends the scope of the rule (even though it is in the rule's intent)

Sounds good. How can I gate it behind preview?

harupy · 2024-11-21T12:59:18Z

I think I've figured it out.

harupy · 2024-11-21T13:08:45Z

@MichaReiser updated the code.

crates/ruff_linter/src/rules/pylint/rules/invalid_envvar_default.rs

harupy · 2024-11-22T13:28:26Z

...s/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW1508_invalid_envvar_default.py.snap

@@ -1,6 +1,5 @@
 ---
 source: crates/ruff_linter/src/rules/pylint/mod.rs
-snapshot_kind: text


Not sure why this was removed.

Can you try updating your local cargo insta version running cargo install cargo-insta --force?

...s/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW1508_invalid_envvar_default.py.snap

…es__pylint__tests__PLW1508_invalid_envvar_default.py.snap

...snapshots/ruff_linter__rules__pylint__tests__preview__PLW1508_invalid_envvar_default.py.snap

…es__pylint__tests__preview__PLW1508_invalid_envvar_default.py.snap

MichaReiser · 2024-11-22T14:55:23Z

The extension seem useful in my view and it makes sense that os.getenv and os.environ.get are treated the same. Considering that os.getenv just delegates to Environ.get (source)

I wonder if there's a reason for pylint not supporting os.environ.get. It was mentioned in the original issue requesting the rule but then never implemented.

I think it would be great to open an upstream issue first before we deviate unnecessarily.

harupy · 2024-11-22T15:22:42Z

Sounds good. Let me file a ticket.

harupy · 2024-11-22T15:32:28Z

Filed pylint-dev/pylint#10092.

MichaReiser · 2024-11-22T16:41:52Z

Thank you @harupy Let's give them a few days. I'm all in for merging if we don't hear back from them in a few days. In case I forget to check in, feel free to ping me!

MichaReiser · 2024-11-22T19:13:51Z

Pylint upstream is okay with supporting this, although they generally recommend using a type checker, which makes sense.

…LW1508`) (#16674) ## Summary This PR stabilizes the new behavior introduced in #14512 to also detect defalut value arguemnts to `os.environ.get` that have an invalid type (not `str`). There's an upstream issue for this behavior change pylint-dev/pylint#10092 that was accepted and a PR, but it hasn't been merged yet. This behavior change was first shipped with Ruff 0.8.1 (Nov 22). There has only be one PR since the new behavior was introduced but it was unrelated to the scope increase (#14841).

Fix PLW1508 to flag os.environ.get

f28e2a8

MichaReiser added rule Implementing or modifying a lint rule preview Related to preview mode features labels Nov 21, 2024

harupy added 2 commits November 21, 2024 22:06

Check os.environ.get if preview

059c600

Add test for preview

26d311c

Fix failed test

3f9b632

harupy commented Nov 21, 2024

View reviewed changes

crates/ruff_linter/src/rules/pylint/rules/invalid_envvar_default.rs Show resolved Hide resolved

harupy commented Nov 22, 2024

View reviewed changes

MichaReiser changed the title ~~Fix invalid-envvar-default (PLW1508) to flag os.environ.get~~ Dedect os.environ.get in invalid-envvar-default (PLW1508) Nov 22, 2024

MichaReiser changed the title ~~Dedect os.environ.get in invalid-envvar-default (PLW1508)~~ Extend invalid-envvar-default (PLW1508) to detect os.environ.get Nov 22, 2024

harupy commented Nov 22, 2024

View reviewed changes

...s/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW1508_invalid_envvar_default.py.snap Show resolved Hide resolved

Update crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rul…

8970a09

…es__pylint__tests__PLW1508_invalid_envvar_default.py.snap

harupy commented Nov 22, 2024

View reviewed changes

...snapshots/ruff_linter__rules__pylint__tests__preview__PLW1508_invalid_envvar_default.py.snap Show resolved Hide resolved

Update crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rul…

7e6558b

…es__pylint__tests__preview__PLW1508_invalid_envvar_default.py.snap

MichaReiser merged commit 931fa06 into astral-sh:main Nov 22, 2024

harupy deleted the fix-PLW1508 branch November 22, 2024 23:39

BrewTestBot mentioned this pull request Nov 29, 2024

ruff 0.8.1 Homebrew/homebrew-core#199449

Merged

MichaReiser mentioned this pull request Mar 12, 2025

[pylint] Detect invalid default value type for os.environ.get (PLW1508) #16674

Merged

Comments

Conversation

harupy commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

github-actions bot commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

harupy commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaReiser commented Nov 21, 2024

Uh oh!

harupy commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

harupy commented Nov 21, 2024

Uh oh!

harupy commented Nov 21, 2024

Uh oh!

Uh oh!

harupy Nov 22, 2024

Choose a reason for hiding this comment

Uh oh!

MichaReiser Nov 22, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MichaReiser commented Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

harupy commented Nov 22, 2024

Uh oh!

harupy commented Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaReiser commented Nov 22, 2024

Uh oh!

MichaReiser commented Nov 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

harupy commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

`ruff-ecosystem` results

harupy commented Nov 21, 2024 •

edited

Loading

harupy commented Nov 21, 2024 •

edited

Loading

MichaReiser commented Nov 22, 2024 •

edited

Loading

harupy commented Nov 22, 2024 •

edited

Loading