Skip to content
New issue

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

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

Already on GitHub? # to your account

[DOC201] Permit explicit None in functions that only return None #13064

Merged
merged 5 commits into from
Aug 27, 2024

Conversation

tjkuson
Copy link
Contributor

@tjkuson tjkuson commented Aug 22, 2024

Summary

No longer reports docstring-missing-returns (DOC201) on explicit returns in functions that only return None.

For example,

def foo(obj: object) -> None:
    """A very helpful docstring.

    Args:
        obj (object): An object.
    """
    if obj is None:
        return
    print(obj)

no longer reports DOC201.

Closes #13062

Methodology

When visiting, count the number of return Nones visited in the body. There are then two scenarios under which the diagnostic is skipped:

  1. If the return is not annotated and if the number of return None to the total number of returns (that is to say, all the paths return None), skip the diagnostic.
  2. If the return is annotated, and that annotation is None, skip the diagnostic.

Return annotations

I saw in the ecosystem check that functions that only return None but annotated otherwise were now being ignored. For example,

def foo(s: str) -> str | None:
    """A very helpful docstring.

    Args:
        s (str): A string.
    """
    return None

would no longer report a diagnostic. I changed this back by always checking the rule if the return annotation is not None: if the return annotation is anything other than None, run the diagnostic.

I think this is more likely to be the expected behaviour, especially due to the functional overlap between docstrings and type annotations. You could argue that checking against the return type annotation should be all this patch does, but that would make the rule quite unhelpful in untyped contexts. For example,

def foo(obj):
    """A very helpful docstring.

    Args:
        obj (object): An object.
    """
    if obj is None:
        return None
    print(obj)

would still report DOC201.

Test Plan

cargo nextest run

@tjkuson tjkuson marked this pull request as ready for review August 22, 2024 22:59
@tjkuson tjkuson changed the title [DOC201] Permit explicit None early returns [DOC201] Permit explicit None in functions that only return None Aug 22, 2024
@tjkuson tjkuson changed the title [DOC201] Permit explicit None in functions that only return None [DOC201] Permit explicit None in functions that only return None Aug 22, 2024
Copy link
Contributor

github-actions bot commented Aug 22, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+0 -67 violations, +0 -0 fixes in 4 projects; 50 projects unchanged)

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

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

- airflow/dag_processing/processor.py:692:9: DOC201 `return` is not documented in docstring
- airflow/io/path.py:386:13: DOC201 `return` is not documented in docstring
- airflow/metrics/datadog_logger.py:117:13: DOC201 `return` is not documented in docstring
- airflow/metrics/datadog_logger.py:138:13: DOC201 `return` is not documented in docstring
- airflow/metrics/datadog_logger.py:76:13: DOC201 `return` is not documented in docstring
- airflow/metrics/datadog_logger.py:96:13: DOC201 `return` is not documented in docstring
- airflow/metrics/statsd_logger.py:109:13: DOC201 `return` is not documented in docstring
- airflow/metrics/statsd_logger.py:125:13: DOC201 `return` is not documented in docstring
- airflow/metrics/statsd_logger.py:139:13: DOC201 `return` is not documented in docstring
- airflow/metrics/statsd_logger.py:94:13: DOC201 `return` is not documented in docstring
- airflow/models/connection.py:182:13: DOC201 `return` is not documented in docstring
- airflow/models/dag.py:1270:9: DOC201 `return` is not documented in docstring
- airflow/models/dag.py:1275:9: DOC201 `return` is not documented in docstring
- airflow/models/dagbag.py:270:13: DOC201 `return` is not documented in docstring
- airflow/models/variable.py:332:13: DOC201 `return` is not documented in docstring
- airflow/providers/airbyte/operators/airbyte.py:126:9: DOC201 `return` is not documented in docstring
- airflow/providers/airbyte/operators/airbyte.py:139:9: DOC201 `return` is not documented in docstring
- airflow/providers/airbyte/sensors/airbyte.py:162:9: DOC201 `return` is not documented in docstring
- airflow/providers/amazon/aws/hooks/s3.py:1327:9: DOC201 `return` is not documented in docstring
- airflow/providers/amazon/aws/sensors/s3.py:407:9: DOC201 `return` is not documented in docstring
- airflow/providers/apache/hive/hooks/hive.py:431:13: DOC201 `return` is not documented in docstring
- airflow/providers/common/compat/openlineage/facet.py:30:5: DOC201 `return` is not documented in docstring
- airflow/providers/google/cloud/hooks/bigquery.py:1295:9: DOC201 `return` is not documented in docstring
- airflow/providers/google/cloud/hooks/bigquery.py:2600:9: DOC201 `return` is not documented in docstring
- airflow/providers/google/cloud/hooks/bigquery.py:2626:9: DOC201 `return` is not documented in docstring
- airflow/providers/google/cloud/hooks/bigquery.py:2639:9: DOC201 `return` is not documented in docstring
- airflow/providers/google/cloud/hooks/bigquery.py:2744:9: DOC201 `return` is not documented in docstring
- airflow/providers/google/cloud/hooks/bigquery.py:2796:9: DOC201 `return` is not documented in docstring
- airflow/providers/google/cloud/hooks/bigquery_dts.py:188:9: DOC201 `return` is not documented in docstring
- airflow/providers/google/cloud/hooks/life_sciences.py:164:17: DOC201 `return` is not documented in docstring
- airflow/providers/google/cloud/hooks/secret_manager.py:345:9: DOC201 `return` is not documented in docstring
- airflow/providers/google/cloud/operators/dataplex.py:1027:9: DOC201 `return` is not documented in docstring
- airflow/providers/google/cloud/operators/dataplex.py:1207:9: DOC201 `return` is not documented in docstring
- airflow/providers/google/cloud/operators/dataplex.py:1594:9: DOC201 `return` is not documented in docstring
- airflow/providers/google/cloud/operators/dataplex.py:1733:9: DOC201 `return` is not documented in docstring
- airflow/providers/google/cloud/operators/dataproc.py:1498:9: DOC201 `return` is not documented in docstring
- airflow/providers/google/cloud/operators/dataproc.py:2647:9: DOC201 `return` is not documented in docstring
- airflow/providers/google/cloud/sensors/gcs.py:551:13: DOC201 `return` is not documented in docstring
... 13 additional changes omitted for project

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

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

- superset/db_engine_specs/base.py:1992:9: DOC201 `return` is not documented in docstring
- superset/db_engine_specs/base.py:2103:9: DOC201 `return` is not documented in docstring
- superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py:105:5: DOC201 `return` is not documented in docstring

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

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

- src/bokeh/application/application.py:234:9: DOC201 `return` is not documented in docstring
- src/bokeh/application/application.py:247:9: DOC201 `return` is not documented in docstring
- src/bokeh/application/handlers/directory.py:254:9: DOC201 `return` is not documented in docstring
- src/bokeh/application/handlers/directory.py:270:9: DOC201 `return` is not documented in docstring
- src/bokeh/application/handlers/lifecycle.py:105:9: DOC201 `return` is not documented in docstring
- src/bokeh/application/handlers/lifecycle.py:115:9: DOC201 `return` is not documented in docstring
- src/bokeh/application/handlers/lifecycle.py:125:9: DOC201 `return` is not documented in docstring
- src/bokeh/application/handlers/lifecycle.py:90:9: DOC201 `return` is not documented in docstring
- src/bokeh/client/websocket.py:71:9: DOC201 `return` is not documented in docstring
- src/bokeh/server/views/ws.py:262:9: DOC201 `return` is not documented in docstring
... 1 additional changes omitted for project

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

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

- zerver/lib/cache.py:261:9: DOC201 `return` is not documented in docstring
- zerver/tests/test_openapi.py:466:13: DOC201 `return` is not documented in docstring

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
DOC201 67 0 67 0 0

@AlexWaygood
Copy link
Member

There's some situations in the ecosystem check here where the diagnostic is going away and it looks like a false negative is introduced, such as https://github.com/apache/airflow/blob/6c878866dedc182c55e18e490bff3f3a9e9c5747/airflow/providers/airbyte/operators/airbyte.py#L126. The reason there for the error going away is that the function is typed as always returning None -- looking at the function, I think it's pretty clear that it doesn't always return None, but I guess that's a bug in their type annotations rather than a bug in the logic of this PR. I think it's correct for us to believe people's type annotations in this rule.

@AlexWaygood AlexWaygood self-assigned this Aug 27, 2024
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@AlexWaygood AlexWaygood added docstring Related to docstring linting or formatting preview Related to preview mode features bug Something isn't working labels Aug 27, 2024
@AlexWaygood AlexWaygood enabled auto-merge (squash) August 27, 2024 15:57
@AlexWaygood AlexWaygood merged commit 96b42b0 into astral-sh:main Aug 27, 2024
18 checks passed
Copy link

codspeed-hq bot commented Aug 27, 2024

CodSpeed Performance Report

Merging #13064 will degrade performances by 9.33%

Comparing tjkuson:doc201-explicit-none (bb48a09) with main (e6d0c4a)

Summary

❌ 1 regressions
✅ 31 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main tjkuson:doc201-explicit-none Change
linter/all-with-preview-rules[numpy/globals.py] 821 µs 905.5 µs -9.33%

@AlexWaygood
Copy link
Member

Merging #13064 will degrade performances by 9.33%

Looks spurious; it's been very unreliable recently sadly

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working docstring Related to docstring linting or formatting preview Related to preview mode features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docstring-missing-returns (DOC201) reports on explicit None returns in bodies that return None only
2 participants