Extend invalid-envvar-default (PLW1508) to detect os.environ.get

[harupy]

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]

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]

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

[MichaReiser]

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]

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]

I think I've figured it out.

[harupy]

@MichaReiser updated the code.

[harupy]

Not sure why this was removed.

[MichaReiser]

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

[MichaReiser]

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]

Sounds good. Let me file a ticket.

[harupy]

Filed pylint-dev/pylint#10092.

[MichaReiser]

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]

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