Skip to content

[flake8-bugbear] Treat raise NotImplemented-only bodies as stub functions#10990

Merged
charliermarsh merged 9 commits intoastral-sh:mainfrom
Philipp-Thiel:B006_stubs
Apr 17, 2024
Merged

[flake8-bugbear] Treat raise NotImplemented-only bodies as stub functions#10990
charliermarsh merged 9 commits intoastral-sh:mainfrom
Philipp-Thiel:B006_stubs

Conversation

@Philipp-Thiel
Copy link
Contributor

Summary

As discussed in #10083 (comment), stubs detection now also covers the case where the function body raises NotImplementedError and does nothing else.

Test Plan

Tests for the relevant cases were added in B006_8.py

@github-actions
Copy link
Contributor

github-actions bot commented Apr 17, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@charliermarsh charliermarsh self-assigned this Apr 17, 2024
Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

Thanks!

@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Apr 17, 2024
@charliermarsh charliermarsh enabled auto-merge (squash) April 17, 2024 13:59
}) => exception.as_ref().is_some_and(|exc| {
semantic
.resolve_builtin_symbol(map_callable(exc))
.is_some_and(|name| matches!(name, "NotImplementedError" | "NotImplemented"))
Copy link
Member

Choose a reason for hiding this comment

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

Changed this to use the semantic model, and allow raise NotImplementedError (without argument) and raise NotImplemented.

Choose a reason for hiding this comment

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

Why do you allow raise NotImplemented? NotImplemented is not an exception. It should never be raised. NotImplemented can be returned by comparison operators.

Copy link
Member

Choose a reason for hiding this comment

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

We have separate rules to guard against raise NotImplemented. But if the entire function is raise NotImplemented, it's almost certainly intended to be raise NotImplementedError, right?

Choose a reason for hiding this comment

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

I see, fair enough!

@charliermarsh charliermarsh changed the title B006 stubs extension to include NotImplementedError [flake8-bugbear] Treat raise NotImplemented-only bodies as stub functions Apr 17, 2024
@charliermarsh charliermarsh disabled auto-merge April 17, 2024 13:59
@charliermarsh charliermarsh enabled auto-merge (squash) April 17, 2024 14:00
@charliermarsh charliermarsh merged commit 2971655 into astral-sh:main Apr 17, 2024


def quux(a: list = []):
raise NotImplemented

Choose a reason for hiding this comment

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

What's the purpose of this example?

Copy link
Member

Choose a reason for hiding this comment

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

To verify that we treat raise NotImplemented like raise NotImplementedError. (Just to be clear, these aren't examples, they're tests.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rule Implementing or modifying a lint rule

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants